Re: [libvirt] [PATCH python] Add missing binding of security model/label APIs

On 11/26/2013 11:32 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > The virNodeGetSecurityModel, virDomainGetSecurityLabel and > virDomainGetSecurityLabelList methods were disabled in the > python binding for inexplicable reasons. > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > --- > +++ b/libvirt-override.c > @@ -2865,6 +2865,83 @@ libvirt_virNodeGetInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { > } > > static PyObject * > +libvirt_virNodeGetSecurityModel(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { { on its own line (Hmm, we aren't very consistent in this file) > + PyObject *py_retval; > + int c_retval; > + virConnectPtr conn; > + PyObject *pyobj_conn; > + virSecurityModel model; > + > + if (!PyArg_ParseTuple(args, (char *)"O:virDomainGetSecurityModel", &pyobj_conn)) > + return NULL; > + conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); > + > + LIBVIRT_BEGIN_ALLOW_THREADS; > + c_retval = virNodeGetSecurityModel(conn, &model); > + LIBVIRT_END_ALLOW_THREADS; > + if (c_retval < 0) > + return VIR_PY_NONE; > + py_retval = PyList_New(2); If PyList_New() fails, py_retval is NULL,... > + PyList_SetItem(py_retval, 0, libvirt_constcharPtrWrap(&model.model[0])); ...and this will crash. Instead, you should just return NULL early. (Then again, we have LOTS of places where we aren't checking the return of PyList_SetItem for failure, which is a major cleanup in its own right). > + PyList_SetItem(py_retval, 1, libvirt_constcharPtrWrap(&model.doi[0])); > + return py_retval; > +} > + > +static PyObject * > +libvirt_virDomainGetSecurityLabel(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { { placement > + py_retval = PyList_New(2); > + PyList_SetItem(py_retval, 0, libvirt_constcharPtrWrap(&label.label[0])); Same comment on error handling. > + PyList_SetItem(py_retval, 1, libvirt_boolWrap(label.enforcing)); > + return py_retval; > +} > + > +#if LIBVIR_CHECK_VERSION(0, 9, 13) > +static PyObject * > +libvirt_virDomainGetSecurityLabelList(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { { placement > + py_retval = PyList_New(0); > + for (i = 0 ; i < c_retval ; i++) { No space before semicolon (hmm, we lost our syntax checker in the split)

> + PyObject *entry = PyList_New(2); > + PyList_SetItem(entry, 0, libvirt_constcharPtrWrap(&labels[i].label[0])); Again, can't libvirt_constcharPtrWrap() return NULL (on OOM situations), which will crash PyList_SetItem()? Only findings are style-related or part of a bigger cleanup needed for proper error handling, so ACK.