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

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(a)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.