[libvirt] [python v2 PATCH] Add dict check for setTime and allow pass one valid parameter

[Eric Blake]

On 10/27/2014 12:58 AM, Luyao Huang wrote:
> When pass None to time, it will set guest time to 0.
> When pass an empty dictionary, it will report error.
> Allow a one-element dictionary which contains 'seconds'
> or 'nseconds', setting JUST seconds will do the sane
> thing of passing 0 for nseconds, instead of erroring out.
> 
> Signed-off-by: Luyao Huang <lhuang at redhat.com>
> ---
>  libvirt-override-virDomain.py |  2 ++
>  libvirt-override.c            | 26 +++++++++++++++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/libvirt-override-virDomain.py b/libvirt-override-virDomain.py
> index a50ec0d..d788657 100644
> --- a/libvirt-override-virDomain.py
> +++ b/libvirt-override-virDomain.py
> @@ -69,6 +69,8 @@
>      def setTime(self, time=None, flags=0):
>          """Set guest time to the given value. @time is a dict conatining

s/conatining/containing/ while you are at it.

>          'seconds' field for seconds and 'nseconds' field for nanosecons """

and s/nanosecons/nanoseconds/

> +        if time is None:
> +            time = {'nseconds': 0, 'seconds': 0L}

I'd rather that we error out for None instead of silently converting to
0. Using all 0 (aka 1970) is usually the wrong thing.

Likewise, while defaulting nseconds to 0 makes sense, I think we should
require seconds to be present.

>          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 a53b46f..5c016f9 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -7798,8 +7798,28 @@ libvirt_virDomainSetTime(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
>      domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
>  
>      py_dict_size = PyDict_Size(py_dict);
> +    
> +    if (py_dict_size == 1) {
> +        PyObject *pyobj_seconds, *pyobj_nseconds;
> +
> +        if ((pyobj_seconds = PyDict_GetItemString(py_dict, "seconds")) &&
> +            (libvirt_longlongUnwrap(pyobj_seconds, &seconds) < 0)) {
> +            PyErr_Format(PyExc_LookupError, "malformed 'seconds'");
> +            goto cleanup;
> +        }
>  
> -    if (py_dict_size == 2) {
> +        if ((pyobj_nseconds = PyDict_GetItemString(py_dict, "nseconds")) &&
> +            (libvirt_uintUnwrap(pyobj_nseconds, &nseconds) < 0)) {
> +            PyErr_Format(PyExc_LookupError, "malformed 'nseconds'");
> +            goto cleanup;
> +        }
> +
> +        if (!pyobj_nseconds && !pyobj_seconds) {
> +            PyErr_Format(PyExc_LookupError, "Dictionary must contain "
> +                     "'seconds' or 'nseconds'");
> +            goto cleanup;
> +        }
> +    } else if (py_dict_size == 2) {
>          PyObject *pyobj_seconds, *pyobj_nseconds;

This feels overly complex.  I think all we really need is:

validate that py_dict is a dict (and not None, per my argument above)
if dict contains seconds:
    populate seconds
else:
    error out
if dict contains nseconds:
    populate nseconds
else:
    nseconds = 0
if dict contains anything else:
    error out

>  
>          if (!(pyobj_seconds = PyDict_GetItemString(py_dict, "seconds")) ||
> @@ -7813,9 +7833,9 @@ libvirt_virDomainSetTime(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) {
>              PyErr_Format(PyExc_LookupError, "malformed or missing 'nseconds'");
>              goto cleanup;
>          }
> -    } else if (py_dict_size > 0) {
> +    } else if (py_dict_size >= 0) {
>          PyErr_Format(PyExc_LookupError, "Dictionary must contain "
> -                     "'seconds' and 'nseconds'");
> +                     "'seconds' or 'nseconds'");

The error message needs touching up if nseconds is going to be optional.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141027/d6903071/attachment-0001.sig>