2
\$\begingroup\$

I have some hardware for which I have a driver for which I have a library which can be used to easily configure and probe the device. Now I want to add support for this library to python because the management interface runs on a web server with a Python back-end, and I want to make the device manageable from the web interface, so it'll be easier if the back-end Python code can speak "directly" to the hardware.

Only issue is that I'm not certain about the ownership semantics in Python (2.7 if it matters, but I suspect that these semantics haven't changed much in 3.x), and this particular device needs to have very long uptime so I want to make sure I'm understanding the reference semantics correctly.

While the primary goal is to make sure I don't have any reference leaks, actually understanding the reference counting system is important to me. So if I'm unnecessarily releasing a reference, then that's interesting to know as well.

Specifically I'm wondering:

  • I assume that in get_conf() I don't need to incref the dict object when it's returned? (I read somewhere that C functions act as regular scopes so new objects are deref'd when the function returns—is this accurate, and does this apply to returned objects?)
  • In set_conf(), do I need to decref the tmpstr? (Related to the point above). Let's say I create tmpstr once, and then reuse it and I don't decref it, will it be decref'd once the function returns?

I'm wondering if I have gotten references figured out correctly.

#include <stdbool.h>

#include <Python.h>

static PyObject *get_conf(PyObject *self, PyObject *args);
static PyObject *set_conf(PyObject *self, PyObject *args);

static PyMethodDef Methods[] = {
    {"get_conf", get_conf, METH_NOARGS,
     "Get configuration as a dictionary."},
    {"set_conf", set_conf, METH_VARARGS,
     "Set configuration from a dictionary."},
    {NULL, NULL, 0, NULL}        /* Sentinel */
};


bool g_enabled = true;
long g_val = 42;

PyMODINIT_FUNC initmyfoo(void)
{
  PyObject *m;

  m = Py_InitModule("myfoo", Methods);
  if(m == NULL) {
    return;
  }
}

static PyObject *get_conf(PyObject *self, PyObject *args)
{
  PyObject *dict;
  PyObject *b;

  dict = PyDict_New();
  if(dict == NULL) {
    /* should set exception before returning NULL! */
    return NULL;
  }

  if(g_enabled) {
    b = Py_True;
  } else {
    b = Py_False;
  }
  PyDict_SetItemString(dict, "enabled", b);
  PyDict_SetItemString(dict, "num_foo", PyLong_FromLong(42));

  return dict;
}

static PyObject *set_conf(PyObject *self, PyObject *args)
{
  PyObject *dict;
  PyObject *tmpstr;

  if(!PyArg_ParseTuple(args, "O", &dict)) {
    return NULL;
  }

  tmpstr = PyString_FromString("enabled");
  if(PyDict_Contains(dict, tmpstr) == 1) {
    PyObject* b;

    b = PyDict_GetItemString(dict, "enabled");
    if(PyObject_IsTrue(b)) {
      puts("enabling!");
      g_enabled = true;
    } else {
      puts("disabling!");
      g_enabled = false;
    }
  }
  Py_DECREF(tmpstr);


  tmpstr = PyString_FromString("num_foo");
  if(PyDict_Contains(dict, tmpstr) == 1) {
    PyObject* i;
    long val;

    i = PyDict_GetItemString(dict, "num_foo");
    if(PyInt_Check(i)) {
      long val;

      val = PyInt_AsLong(i);
      printf("setting num_foo=%u\n", (unsigned)val);

      g_val = val;
    } else {
      /* ToDo: Not a number exception */
      return NULL;
    }
  }
  Py_DECREF(tmpstr);


  Py_RETURN_NONE;
}
\$\endgroup\$
3
\$\begingroup\$

Just reviewing get_conf.

  1. Support for Python 2.7 is planned to end in 2020, so it is not a good time to be starting new projects that run on Python 2.7 only.

  2. The line:

    PyDict_SetItemString(dict, "num_foo", PyLong_FromLong(42));
    

    has three problems! First, PyLong_FromLong can fail (returning NULL), but your code does not check for this. Second, PyLong_FromLong returns a "new reference" which you never decrement, so this object will leak. Third, PyDict_SetItemString can also fail (returning -1), but your code does not check for this.

    So, inconvenient as it may seem, you must write something like:

    PyObject *num_foo = PyLong_FromLong(42);
    if (num_foo == NULL) {
        /* handle failure somehow */
    }
    if (PyDict_SetItemString(dict, "num_foo", num_foo) != 0) {
        /* handle failure somehow */
    }
    Py_DECREF(num_foo);
    
  3. One approach to writing a function that has many error cases is to have an error-handling section which is responsible for decrementing all the references that were acquired by the code. The error handlers should be arranged in reverse order, like this:

    static PyObject *get_conf(PyObject *self, PyObject *args)
    {
        PyObject *dict = PyDict_New(); /* (1) */
        if (dict == NULL) {
            goto fail_dict;
        }
        PyObject *enabled = g_enabled ? Py_True : Py_False;
        if (PyDict_SetItemString(dict, "enabled", enabled) != 0) {
            goto fail_set_enabled;
        }
        PyObject *num_foo = PyLong_FromLong(42);
        if (num_foo == NULL) {
            goto fail_num_foo;
        }
        if (PyDict_SetItemString(dict, "num_foo", num_foo) != 0) {
            goto fail_set_num_foo;
        }
        Py_DECREF(num_foo);
        return dict; /* (2) */
    
    fail_set_num_foo:
        Py_DECREF(num_foo);    
    fail_num_foo:
    fail_set_enabled:
        Py_DECREF(dict); /* (3) */
    fail_dict:
        return NULL;
    }
    

    You can then check the reference-counting behaviour for each object. For example, the dict object gets one reference at (1), which is either returned to the caller at (2), or else decremented at (3). So this object does not leak.

  4. The general inconvenience and delicacy of reference counting in C extension modules suggests that you might be better off with a hybrid approach where you implement a minimal hardware interface layer in C, and then package this up in Python to provide a friendlier interface.

    The way this would work is that in C you would make a module _myfoo which contains functions like this:

    static PyObject *get_enabled(PyObject *self, PyObject *args)
    {
        return PyBool_FromLong(g_enabled);
    }
    
    static PyObject *get_num_foo(PyObject *self, PyObject *args)
    {
        return PyLong_FromLong(g_val);
    }
    

    which are so simple that there is little risk of getting the reference counting wrong, and then in Python you would write a module myfoo containing:

    import _myfoo
    
    def get_conf():
        """Return hardware configuration dictionary with keys:
    
        enabled: bool -- [documentation here]
        num_foo: int -- [documentation here]
        """
        return dict(enabled=_myfoo.get_enabled(),
                    num_foo=_myfoo.get_num_foo())
    

    which you can see is much simpler than trying to allocate a dictionary in C.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.