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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(-)
>
> diff --git a/libvirt-override.c b/libvirt-override.c
> index 84a5436..579ea43 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -58,18 +58,17 @@ extern void initcygvirtmod(void);
>  #define VIR_PY_INT_FAIL (libvirt_intWrap(-1))
>  #define VIR_PY_INT_SUCCESS (libvirt_intWrap(0))
>
> -/* We don't want to free() returned value. As written in doc:
> - * PyString_AsString returns pointer to 'internal buffer of string,
> - * not a copy' and 'It must not be deallocated'. */
>  static char *py_str(PyObject *obj)
>  {
>      PyObject *str = PyObject_Str(obj);
> +    char *ret;
>      if (!str) {
>          PyErr_Print();
>          PyErr_Clear();
>          return NULL;
>      };
> -    return PyString_AsString(str);
> +    libvirt_charPtrUnwrap(str, &ret);
> +    return ret;
>  }
>
>  /* Helper function to convert a virTypedParameter output array into a
> @@ -147,9 +146,8 @@ cleanup:
>  /* Allocate a new typed parameter array with the same contents and
>   * length as info, and using the array params of length nparams as
>   * hints on what types to use when creating the new array. The caller
> - * must NOT clear the array before freeing it, as it points into info
> - * rather than allocated strings.  Return NULL on failure, after
> - * raising a python exception.  */
> + * must clear the array before freeing it. Return NULL on failure,
> + * after raising a python exception.  */
>  static virTypedParameterPtr ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
>  setPyVirTypedParameter(PyObject *info,
>                         const virTypedParameter *params, int nparams)
> @@ -183,7 +181,8 @@ setPyVirTypedParameter(PyObject *info,
>      while (PyDict_Next(info, &pos, &key, &value)) {
>          char *keystr = NULL;
>
> -        if ((keystr = PyString_AsString(key)) == NULL)
> +        if (libvirt_charPtrUnwrap(key, &keystr) < 0 ||
> +            keystr == NULL)
>              goto cleanup;
>
>          for (i = 0; i < nparams; i++) {
> @@ -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:
> @@ -237,8 +238,9 @@ setPyVirTypedParameter(PyObject *info,
>          }
>          case VIR_TYPED_PARAM_STRING:
>          {
> -            char *string_val = PyString_AsString(value);
> -            if (!string_val)
> +            char *string_val;
> +            if (libvirt_charPtrUnwrap(value, &string_val) < 0 ||
> +                !string_val)
>                  goto cleanup;
>              temp->value.s = string_val;
>              break;
> @@ -297,6 +299,7 @@ virPyDictToTypedParams(PyObject *dict,
>      int n = 0;
>      int max = 0;
>      int ret = -1;
> +    char *keystr = NULL;
>
>      *ret_params = NULL;
>      *ret_nparams = 0;
> @@ -305,10 +308,10 @@ virPyDictToTypedParams(PyObject *dict,
>          return -1;
>
>      while (PyDict_Next(dict, &pos, &key, &value)) {
> -        char *keystr;
>          int type = -1;
>
> -        if (!(keystr = PyString_AsString(key)))
> +        if (libvirt_charPtrUnwrap(key, &keystr) < 0 ||
> +            !keystr)
>              goto cleanup;
>
>          for (i = 0; i < nhints; i++) {
> @@ -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.

>                  goto cleanup;
> +            }
> +            VIR_FREE(val);
>              break;
>          }
>          case VIR_TYPED_PARAM_LAST:
>              break; /* unreachable */
>          }
> +        VIR_FREE(keystr);
>      }
>
>      *ret_params = params;
> @@ -413,6 +421,7 @@ virPyDictToTypedParams(PyObject *dict,
>      ret = 0;
>
>  cleanup:
> +    VIR_FREE(keystr);
>      virTypedParamsFree(params, n);
>      return ret;
>  }
> @@ -957,7 +966,7 @@ libvirt_virDomainSetSchedulerParameters(PyObject *self ATTRIBUTE_UNUSED,
>
>  cleanup:
>      virTypedParamsFree(params, nparams);
> -    VIR_FREE(new_params);
> +    virTypedParamsFree(new_params, nparams);
>      return ret;
>  }
>
> @@ -1033,7 +1042,7 @@ libvirt_virDomainSetSchedulerParametersFlags(PyObject *self ATTRIBUTE_UNUSED,
>
>  cleanup:
>      virTypedParamsFree(params, nparams);
> -    VIR_FREE(new_params);
> +    virTypedParamsFree(new_params, nparams);
>      return ret;
>  }
>
> @@ -1107,7 +1116,7 @@ libvirt_virDomainSetBlkioParameters(PyObject *self ATTRIBUTE_UNUSED,
>
>  cleanup:
>      virTypedParamsFree(params, nparams);
> -    VIR_FREE(new_params);
> +    virTypedParamsFree(new_params, nparams);
>      return ret;
>  }
>
> @@ -1227,7 +1236,7 @@ libvirt_virDomainSetMemoryParameters(PyObject *self ATTRIBUTE_UNUSED,
>
>  cleanup:
>      virTypedParamsFree(params, nparams);
> -    VIR_FREE(new_params);
> +    virTypedParamsFree(new_params, nparams);
>      return ret;
>  }
>
> @@ -1347,7 +1356,7 @@ libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
>
>  cleanup:
>      virTypedParamsFree(params, nparams);
> -    VIR_FREE(new_params);
> +    virTypedParamsFree(new_params, nparams);
>      return ret;
>  }
>
> @@ -1468,7 +1477,7 @@ libvirt_virDomainSetInterfaceParameters(PyObject *self ATTRIBUTE_UNUSED,
>
>  cleanup:
>      virTypedParamsFree(params, nparams);
> -    VIR_FREE(new_params);
> +    virTypedParamsFree(new_params, nparams);
>      return ret;
>  }
>
> @@ -2163,9 +2172,9 @@ static int virConnectCredCallbackWrapper(virConnectCredentialPtr cred,
>              pycreditem = PyTuple_GetItem(pycred, i);
>              pyresult = PyList_GetItem(pycreditem, 4);
>              if (pyresult != Py_None)
> -                result = PyString_AsString(pyresult);
> +                libvirt_charPtrUnwrap(pyresult, &result);
>              if (result != NULL) {
> -                cred[i].result = strdup(result);
> +                cred[i].result = result;
>                  cred[i].resultlen = strlen(result);
>              } else {
>                  cred[i].result = NULL;
> @@ -2942,7 +2951,7 @@ libvirt_virDomainGetSecurityLabelList(PyObject *self ATTRIBUTE_UNUSED, PyObject
>          PyObject *entry = PyList_New(2);
>          PyList_SetItem(entry, 0, libvirt_constcharPtrWrap(&labels[i].label[0]));
>          PyList_SetItem(entry, 1, libvirt_boolWrap(labels[i].enforcing));
> -       PyList_Append(py_retval, entry);
> +        PyList_Append(py_retval, entry);
>      }
>      free(labels);
>      return py_retval;
> @@ -4566,10 +4575,11 @@ libvirt_virConnectBaselineCPU(PyObject *self ATTRIBUTE_UNUSED,
>      PyObject *list;
>      virConnectPtr conn;
>      unsigned int flags;
> -    const char **xmlcpus = NULL;
> +    char **xmlcpus = NULL;
>      int ncpus = 0;
>      char *base_cpu;
>      PyObject *pybase_cpu;
> +    size_t i, j;
>
>      if (!PyArg_ParseTuple(args, (char *)"OOi:virConnectBaselineCPU",
>                            &pyobj_conn, &list, &flags))
> @@ -4577,15 +4587,15 @@ libvirt_virConnectBaselineCPU(PyObject *self ATTRIBUTE_UNUSED,
>      conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
>
>      if (PyList_Check(list)) {
> -        size_t i;
> -
>          ncpus = PyList_Size(list);
>          if (VIR_ALLOC_N(xmlcpus, ncpus) < 0)
>              return VIR_PY_NONE;
>
>          for (i = 0; i < ncpus; i++) {
> -            xmlcpus[i] = PyString_AsString(PyList_GetItem(list, i));
> -            if (xmlcpus[i] == NULL) {
> +            if (libvirt_charPtrUnwrap(PyList_GetItem(list, i), &(xmlcpus[i])) < 0 ||
> +                xmlcpus[i] == NULL) {
> +                for (j = 0 ; j < i ; j++)
> +                    VIR_FREE(xmlcpus[j]);
>                  VIR_FREE(xmlcpus);
>                  return VIR_PY_NONE;
>              }
> @@ -4593,9 +4603,11 @@ libvirt_virConnectBaselineCPU(PyObject *self ATTRIBUTE_UNUSED,
>      }
>
>      LIBVIRT_BEGIN_ALLOW_THREADS;
> -    base_cpu = virConnectBaselineCPU(conn, xmlcpus, ncpus, flags);
> +    base_cpu = virConnectBaselineCPU(conn, (const char **)xmlcpus, ncpus, flags);
>      LIBVIRT_END_ALLOW_THREADS;
>
> +    for (i = 0 ; i < ncpus ; i++)
> +        VIR_FREE(xmlcpus[i]);
>      VIR_FREE(xmlcpus);
>
>      if (base_cpu == NULL)
> @@ -4821,7 +4833,7 @@ libvirt_virDomainSetBlockIoTune(PyObject *self ATTRIBUTE_UNUSED,
>
>  cleanup:
>      virTypedParamsFree(params, nparams);
> -    VIR_FREE(new_params);
> +    virTypedParamsFree(new_params, nparams);
>      return ret;
>  }
>
> @@ -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.

>
>  #define NAME(fn) ( fn ## Name ? fn ## Name : # fn )
>
> @@ -5381,6 +5393,12 @@ libvirt_virEventRegisterImpl(ATTRIBUTE_UNUSED PyObject * self,
>      Py_XDECREF(addTimeoutObj);
>      Py_XDECREF(updateTimeoutObj);
>      Py_XDECREF(removeTimeoutObj);
> +    free(addHandleName);
> +    free(updateHandleName);
> +    free(removeHandleName);
> +    free(addTimeoutName);
> +    free(updateTimeoutName);
> +    free(removeTimeoutName);

More free() vs VIR_FREE().
>
>      /* Parse and check arguments */
>      if (!PyArg_ParseTuple(args, (char *) "OOOOOO:virEventRegisterImpl",
> @@ -7084,7 +7102,7 @@ libvirt_virNodeSetMemoryParameters(PyObject *self ATTRIBUTE_UNUSED,
>
>  cleanup:
>      virTypedParamsFree(params, nparams);
> -    VIR_FREE(new_params);
> +    virTypedParamsFree(new_params, nparams);
>      return ret;
>  }
>
> diff --git a/typewrappers.c b/typewrappers.c
> index 1622986..1e99554 100644
> --- a/typewrappers.c
> +++ b/typewrappers.c
> @@ -317,6 +317,24 @@ libvirt_boolUnwrap(PyObject *obj, bool *val)
>      return 0;
>  }
>
> +int
> +libvirt_charPtrUnwrap(PyObject *obj, char **str)
> +{
> +    const char *ret;
> +    *str = NULL;
> +    if (!obj) {
> +        PyErr_SetString(PyExc_TypeError, "unexpected type");
> +        return -1;
> +    }
> +
> +    ret = PyString_AsString(obj);
> +    if (ret &&
> +        !(*str = strdup(ret)))
> +        return -1;
> +
> +    return 0;
> +}
> +
>  PyObject *
>  libvirt_virDomainPtrWrap(virDomainPtr node)
>  {
> diff --git a/typewrappers.h b/typewrappers.h
> index 04e364f..7068426 100644
> --- a/typewrappers.h
> +++ b/typewrappers.h
> @@ -173,6 +173,7 @@ int libvirt_longlongUnwrap(PyObject *obj, long long *val);
>  int libvirt_ulonglongUnwrap(PyObject *obj, unsigned long long *val);
>  int libvirt_doubleUnwrap(PyObject *obj, double *val);
>  int libvirt_boolUnwrap(PyObject *obj, bool *val);
> +int libvirt_charPtrUnwrap(PyObject *obj, char **str);
>  PyObject * libvirt_virConnectPtrWrap(virConnectPtr node);
>  PyObject * libvirt_virDomainPtrWrap(virDomainPtr node);
>  PyObject * libvirt_virNetworkPtrWrap(virNetworkPtr node);
> --
> 1.8.3.1
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Some minor nits but overall looks ok. I'll double check it for any
leaks with more context after I review the rest of the series.

-- 
Doug Goldstein

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]