Re: [PATCH python 06/14] override: Replace PyString_AsString with libvirt_charPtrUnwrap

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

On Mon, Dec 09, 2013 at 10:33:25AM -0600, Doug Goldstein wrote:
> On Mon, Dec 9, 2013 at 9:15 AM, Daniel P. Berrange <berrange@xxxxxxxxxx> wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> >
> > Replace calls to PyString_AsString with the helper method
> > libvirt_charPtrUnwrap. This isolates the code that will
> > change in Python3.
> >
> > In making this change, all callers now have responsibility
> > for free'ing the string.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> > ---
> >  libvirt-override.c | 106 +++++++++++++++++++++++++++++++----------------------
> >  typewrappers.c     |  18 +++++++++
> >  typewrappers.h     |   1 +
> >  3 files changed, 81 insertions(+), 44 deletions(-)

> > @@ -194,12 +193,14 @@ setPyVirTypedParameter(PyObject *info,
> >              PyErr_Format(PyExc_LookupError,
> >                           "Attribute name \"%s\" could not be recognized",
> >                           keystr);
> > +            free(keystr);
> 
> Here you use free() but....
> 
> >              goto cleanup;
> >          }
> >
> >          strncpy(temp->field, keystr, sizeof(*temp->field) - 1);
> >          temp->field[sizeof(*temp->field) - 1] = '\0';
> >          temp->type = params[i].type;
> > +        free(keystr);
> >
> >          switch (params[i].type) {
> >          case VIR_TYPED_PARAM_INT:

> > @@ -396,15 +399,20 @@ virPyDictToTypedParams(PyObject *dict,
> >          }
> >          case VIR_TYPED_PARAM_STRING:
> >          {
> > -            char *val = PyString_AsString(value);
> > -            if (!val ||
> > -                virTypedParamsAddString(&params, &n, &max, keystr, val) < 0)
> > +            char *val;;
> > +            if (libvirt_charPtrUnwrap(value, &val) < 0 ||
> > +                !val ||
> > +                virTypedParamsAddString(&params, &n, &max, keystr, val) < 0) {
> > +                VIR_FREE(val);
> 
> Here you use VIR_FREE(). We should probably be consistent.

Yes, will fix to use VIR_FREE everywhere.


> > @@ -5105,18 +5117,18 @@ libvirt_virConnectDomainEventDeregister(ATTRIBUTE_UNUSED PyObject * self,
> >  /*******************************************
> >   * Event Impl
> >   *******************************************/
> > -static PyObject *addHandleObj     = NULL;
> > -static char *addHandleName        = NULL;
> > -static PyObject *updateHandleObj  = NULL;
> > -static char *updateHandleName     = NULL;
> > -static PyObject *removeHandleObj  = NULL;
> > -static char *removeHandleName     = NULL;
> > -static PyObject *addTimeoutObj    = NULL;
> > -static char *addTimeoutName       = NULL;
> > -static PyObject *updateTimeoutObj = NULL;
> > -static char *updateTimeoutName    = NULL;
> > -static PyObject *removeTimeoutObj = NULL;
> > -static char *removeTimeoutName    = NULL;
> > +static PyObject *addHandleObj;
> > +static char *addHandleName;
> > +static PyObject *updateHandleObj;
> > +static char *updateHandleName;
> > +static PyObject *removeHandleObj;
> > +static char *removeHandleName;
> > +static PyObject *addTimeoutObj;
> > +static char *addTimeoutName;
> > +static PyObject *updateTimeoutObj;
> > +static char *updateTimeoutName;
> > +static PyObject *removeTimeoutObj;
> > +static char *removeTimeoutName;
> 
> Not sure the advantage of this change.

Static variables are always NULL initially.

Regards,
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