diff mbox

[v2,5/5] qmp: Add example usage of strto*l() qemu wrapper

Message ID 5b903011386171258cca188ac2b35ae320cdb340.1437263259.git.carlos.torres@rackspace.com
State New
Headers show

Commit Message

Carlos L. Torres July 18, 2015, 11:52 p.m. UTC 
From: "Carlos L. Torres" <carlos.torres@rackspace.com>

Signed-off-by: Carlos L. Torres <carlos.torres@rackspace.com>
---
 qmp.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Comments

Paolo Bonzini July 19, 2015, 5:51 p.m. UTC | #1 
On 19/07/2015 01:52, Carlos L. Torres wrote:
> +    int err;
>  
>      info->qemu = g_new0(VersionTriple, 1);
> -    info->qemu->major = strtol(version, &tmp, 10);
> +    err = qemu_strtol(version, &tmp, 10, &(info->qemu->major));

There are usually no parentheses around the argument of the & operator.

> +    if (err) {
> +        error_setg(errp, "There was a problem retrieving QEMU major version.");
> +    }

I think it's okay to just assert that err is zero.  Otherwise, this
simple example is okay.  Thanks!

Paolo

>      tmp++;
> -    info->qemu->minor = strtol(tmp, &tmp, 10);
> +
> +    err = qemu_strtol(tmp, &tmp, 10, &(info->qemu->minor));
> +    if (err) {
> +        error_setg(errp, "There was a problem retrieving QEMU minor version.");
> +    }
>      tmp++;
> -    info->qemu->micro = strtol(tmp, &tmp, 10);
> +
> +    err = qemu_strtol(tmp, &tmp, 10, &(info->qemu->micro));
> +    if (err) {
> +        error_setg(errp, "There was a problem retrieving QEMU micro version.");
> +    }
Carlos L. Torres July 19, 2015, 6:27 p.m. UTC | #2 
On Sun, Jul 19, 2015 at 07:51:00PM +0200, Paolo Bonzini wrote:
> 
> 
> On 19/07/2015 01:52, Carlos L. Torres wrote:
> > +    int err;
> >  
> >      info->qemu = g_new0(VersionTriple, 1);
> > -    info->qemu->major = strtol(version, &tmp, 10);
> > +    err = qemu_strtol(version, &tmp, 10, &(info->qemu->major));
> 
> There are usually no parentheses around the argument of the & operator.
> 
> > +    if (err) {
> > +        error_setg(errp, "There was a problem retrieving QEMU major version.");
> > +    }
> 
> I think it's okay to just assert that err is zero.  Otherwise, this
> simple example is okay.  Thanks!
> 
> Paolo
> 
> >      tmp++;
> > -    info->qemu->minor = strtol(tmp, &tmp, 10);
> > +
> > +    err = qemu_strtol(tmp, &tmp, 10, &(info->qemu->minor));
> > +    if (err) {
> > +        error_setg(errp, "There was a problem retrieving QEMU minor version.");
> > +    }
> >      tmp++;
> > -    info->qemu->micro = strtol(tmp, &tmp, 10);
> > +
> > +    err = qemu_strtol(tmp, &tmp, 10, &(info->qemu->micro));
> > +    if (err) {
> > +        error_setg(errp, "There was a problem retrieving QEMU micro version.");
> > +    }

Hi Paolo,

Thanks for taking the time to review the patch.
I'll make those small changes, and post v3 version of the patch.

Thanks,
-- Carlos Torres
diff mbox

Patch

diff --git a/qmp.c b/qmp.c
index 403805a..837cb3f 100644
--- a/qmp.c
+++ b/qmp.c
@@ -49,14 +49,27 @@  VersionInfo *qmp_query_version(Error **errp)
 {
     VersionInfo *info = g_new0(VersionInfo, 1);
     const char *version = QEMU_VERSION;
-    char *tmp;
+    const char *tmp;
+    int err;
 
     info->qemu = g_new0(VersionTriple, 1);
-    info->qemu->major = strtol(version, &tmp, 10);
+    err = qemu_strtol(version, &tmp, 10, &(info->qemu->major));
+    if (err) {
+        error_setg(errp, "There was a problem retrieving QEMU major version.");
+    }
     tmp++;
-    info->qemu->minor = strtol(tmp, &tmp, 10);
+
+    err = qemu_strtol(tmp, &tmp, 10, &(info->qemu->minor));
+    if (err) {
+        error_setg(errp, "There was a problem retrieving QEMU minor version.");
+    }
     tmp++;
-    info->qemu->micro = strtol(tmp, &tmp, 10);
+
+    err = qemu_strtol(tmp, &tmp, 10, &(info->qemu->micro));
+    if (err) {
+        error_setg(errp, "There was a problem retrieving QEMU micro version.");
+    }
+
     info->package = g_strdup(QEMU_PKGVERSION);
 
     return info;