On Tue, Nov 26, 2013 at 01:17:16PM -0700, Eric Blake wrote:
> On 11/26/2013 11:32 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> >
> > The virNodeGetSecurityModel, virDomainGetSecurityLabel and
> > virDomainGetSecurityLabelList methods were disabled in the
> > python binding for inexplicable reasons.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> > ---
>
> > +++ 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)
Yeah, I've tried to get maint.mk / cfg.mk working with a trivial GNUmakefile,
but it seems they rely on a number of make variables set by configure which
we obviously don't have available. So I failed in this attempt. Perhaps you'll
have more luck with better understanding of GNULIB
>
> > + 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.
Yeah, the python binding is pretty awful at this - we need a major cleanup
to make the binding OOM-safe.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list