[libvirt] [libvirt-python][PATCH] Add dict check for setTime and allow pass 'seconds' parameter

[Pavel Hrdina]

On 11/11/2014 10:50 AM, Michal Privoznik wrote:
> From: Luyao Huang <lhuang at redhat.com>
>
> When pass None or a empty dictionary to time, it will report
> error. This commit allows a one-element dictionary which contains
> just 'seconds' field, which results in the same as passing 0 for
> 'nseconds' field. Moreover, dict is checked for unknown fields.
>
> Signed-off-by: Luyao Huang <lhuang at redhat.com>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>   libvirt-override-virDomain.py |  4 ++--
>   libvirt-override.c            | 39 +++++++++++++++++++++++----------------
>   2 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/libvirt-override-virDomain.py b/libvirt-override-virDomain.py
> index a50ec0d..fa5f75f 100644
> --- a/libvirt-override-virDomain.py
> +++ b/libvirt-override-virDomain.py
> @@ -67,8 +67,8 @@
>           return ret
>
>       def setTime(self, time=None, flags=0):
> -        """Set guest time to the given value. @time is a dict conatining
> -        'seconds' field for seconds and 'nseconds' field for nanosecons """
> +        """Set guest time to the given value. @time is a dict containing
> +        'seconds' field for seconds and 'nseconds' field for nanoseconds """
>           ret = libvirtmod.virDomainSetTime(self._o, time, flags)
>           if ret == -1: raise libvirtError ('virDomainSetTime() failed', dom=self)
>           return ret
> diff --git a/libvirt-override.c b/libvirt-override.c
> index c01e52f..57f0ccf 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -7785,12 +7785,14 @@ static PyObject *
>   libvirt_virDomainSetTime(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
>       PyObject *py_retval = NULL;
>       PyObject *pyobj_domain;
> +    PyObject *pyobj_seconds;
> +    PyObject *pyobj_nseconds;
>       PyObject *py_dict;
>       virDomainPtr domain;
>       long long seconds = 0;
>       unsigned int nseconds = 0;
>       unsigned int flags;
> -    ssize_t py_dict_size;
> +    ssize_t py_dict_size = 0;
>       int c_retval;
>
>       if (!PyArg_ParseTuple(args, (char*)"OOI:virDomainSetTime",
> @@ -7798,26 +7800,31 @@ libvirt_virDomainSetTime(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
>           return NULL;
>       domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
>
> -    py_dict_size = PyDict_Size(py_dict);
> -
> -    if (py_dict_size == 2) {
> -        PyObject *pyobj_seconds, *pyobj_nseconds;
> -
> -        if (!(pyobj_seconds = PyDict_GetItemString(py_dict, "seconds")) ||
> -            (libvirt_longlongUnwrap(pyobj_seconds, &seconds) < 0)) {
> -            PyErr_Format(PyExc_LookupError, "malformed or missing 'seconds'");
> +    if (PyDict_Check(py_dict)) {
> +        py_dict_size = PyDict_Size(py_dict);
> +        if ((pyobj_seconds = PyDict_GetItemString(py_dict, "seconds"))) {
> +            if (libvirt_longlongUnwrap(pyobj_seconds, &seconds) < 0) {
> +                PyErr_Format(PyExc_LookupError, "malformed 'seconds'");
> +                goto cleanup;
> +            }
> +        } else {
> +            PyErr_Format(PyExc_LookupError, "Dictionary must contains 'seconds'");
>               goto cleanup;
>           }
>
> -        if (!(pyobj_nseconds = PyDict_GetItemString(py_dict, "nseconds")) ||
> -            (libvirt_uintUnwrap(pyobj_nseconds, &nseconds) < 0)) {
> -            PyErr_Format(PyExc_LookupError, "malformed or missing 'nseconds'");
> +        if ((pyobj_nseconds = PyDict_GetItemString(py_dict, "nseconds"))) {
> +            if (libvirt_uintUnwrap(pyobj_nseconds, &nseconds) < 0) {
> +                PyErr_Format(PyExc_LookupError, "malformed 'nseconds'");
> +                goto cleanup;
> +            }
> +        } else if (py_dict_size > 1) {
> +            PyErr_Format(PyExc_LookupError, "Dictionary contains unknown key");
>               goto cleanup;
>           }
> -    } else if (py_dict_size > 0) {
> -        PyErr_Format(PyExc_LookupError, "Dictionary must contain "
> -                     "'seconds' and 'nseconds'");
> -        goto cleanup;
> +    } else if (py_dict != Py_None || !flags) {
> +        PyErr_Format(PyExc_TypeError, "time must be a dictionary "
> +                     "or None with flags set");
> +        return NULL;

You are returning NULL here which is fine and correct, but to keep the
code consistent either use "goto cleanup" here or change all the other
cases to "return NULL". I'll personally go with "return NULL" and remove
the unnecessary cleanup as it just returns NULL or py_int.

ACK with that change,

Pavel

>       }
>
>       LIBVIRT_BEGIN_ALLOW_THREADS;
>