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(¶ms, &n, &max, keystr, val) < 0)
> + char *val;;
> + if (libvirt_charPtrUnwrap(value, &val) < 0 ||
> + !val ||
> + virTypedParamsAddString(¶ms, &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