[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/2] virsh: Change option parsing functions to return tri-state information.



On 03/08/2011 09:29 AM, Michal Privoznik wrote:
> This is needed to detect situations when optional argument was
> specified with non-integer value: '--int-opt foo'. To keep functions
> uniform vshCommandOptString function was also changed, because it
> returns tri-state value as well. Given result pointer is updated only
> in case of success. If parsing fails, result is not updated at all.
> ---
>  tools/virsh.c |  613 ++++++++++++++++++++++++++-------------------------------
>  1 files changed, 280 insertions(+), 333 deletions(-)

Git complained about some added whitespace; 'make syntax-check' would
have caught that in advance.

But we finally arrived at my vision of making these parse functions
useful!  And the diffstat shows a net reduction in lines!

> @@ -781,7 +779,7 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          return FALSE;
>  
> -    name = vshCommandOptString(cmd, "devname", NULL);
> +    vshCommandOptString(cmd, "devname", &name);

Hmm, so you didn't add the ATTRIBUTE_RETURN_CHECK, after all.  I guess
that's okay for now.

> @@ -2300,7 +2287,10 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd)
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          return FALSE;
>  
> -    cell = vshCommandOptInt(cmd, "cellno", &cell_given);
> +    if ( (cell_given = vshCommandOptInt(cmd, "cellno", &cell)) < 0) {
> +        vshError(ctl, "%s", _("cell number has to be a number"));
> +        goto cleanup;
> +    }

Ouch - you didn't pre-initialize cell, but there is still a codepath
where you check if (cell == -1).  Too bad gcc didn't flag it.

> @@ -2862,7 +2850,7 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          return FALSE;
>  
> -    count = vshCommandOptInt(cmd, "count", &count);
> +    vshCommandOptInt(cmd, "count", &count);

We really should be adding ATTRIBUTE_RETURN_CHECK, and flagging parse
errors.  Otherwise, you end up using uninitialized data for count on a
parse error.

Basically, anywhere we used to give an error for !found, we now give an
error for result <= 0; and anywhere we didn't care about found, we now
give an error for result < 0.

> @@ -2984,7 +2976,11 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          return FALSE;
>  
> -    kilobytes = vshCommandOptInt(cmd, "kilobytes", &kilobytes);
> +    if (vshCommandOptInt(cmd, "kilobytes", &kilobytes) < 0) {
> +        vshError(ctl, "%s", _("memory sizehas to be a number"));

s/sizehas/size has/

> @@ -3055,23 +3051,19 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>          return FALSE;
>  
> -    hard_limit =
> -        vshCommandOptLongLong(cmd, "hard-limit", NULL);
> +    vshCommandOptLongLong(cmd, "hard-limit", (unsigned long long*) &hard_limit);

Yuck - why are we casting here?  Oh, because you set
vshCommandOptLongLong to take ull* instead of ll*.

>  
>  /*
> - * Returns option as INT
> + * @cmd command reference
> + * @name option name
> + * @value result
> + *
> + * Convert option to int
> + * Return value:
> + * >0 if option found and valid (@value updated)
> + * 0 in case option not found (@value untouched)
> + * <0 in all other cases (@value untouched)
>   */
>  static int
> -vshCommandOptInt(const vshCmd *cmd, const char *name, int *found)
> +vshCommandOptInt(const vshCmd *cmd, const char *name, int *value)
>  {
>      vshCmdOpt *arg = vshCommandOpt(cmd, name);
> -    int res = 0, num_found = FALSE;
> +    int ret = 0, num;
>      char *end_p = NULL;
>  
>      if ((arg != NULL) && (arg->data != NULL)) {
> -        res = strtol(arg->data, &end_p, 10);
> -        if ((arg->data == end_p) || (*end_p!= 0))
> -            num_found = FALSE;
> -        else
> -            num_found = TRUE;
> +        num = strtol(arg->data, &end_p, 10);
> +        ret = -1;
> +        if ((arg->data != end_p) && (*end_p == 0) && value) {

This check for non-NULL value is redundant, given that you already used
the compiler ATTRIBUTE_NONNULL to guarantee that (at least, for decently
smart compilers; it's a shame that gcc only warns for literal NULL and
that you have to use clang to catch other null parameters via data-flow
checks).

Here's what I'm squashing in before pushing:

diff --git i/tools/virsh.c w/tools/virsh.c
index d4d0949..14c6d6b 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -253,14 +253,16 @@ static int vshCmdGrpHelp(vshControl *ctl, const
char *name);

 static vshCmdOpt *vshCommandOpt(const vshCmd *cmd, const char *name);
 static int vshCommandOptInt(const vshCmd *cmd, const char *name, int
*value)
-    ATTRIBUTE_NONNULL(3);
+    ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
 static int vshCommandOptUL(const vshCmd *cmd, const char *name,
-                           unsigned long *value) ATTRIBUTE_NONNULL(3);
+                           unsigned long *value)
+    ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
 static int vshCommandOptString(const vshCmd *cmd, const char *name,
-                               const char **value) ATTRIBUTE_NONNULL(3);
+                               const char **value)
+    ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
 static int vshCommandOptLongLong(const vshCmd *cmd, const char *name,
-                                 unsigned long long *value)
-    ATTRIBUTE_NONNULL(3);
+                                 long long *value)
+    ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
 static int vshCommandOptBool(const vshCmd *cmd, const char *name);
 static char *vshCommandOptArgv(const vshCmd *cmd, int count);

@@ -780,7 +782,8 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         return FALSE;

-    vshCommandOptString(cmd, "devname", &name);
+    if (vshCommandOptString(cmd, "devname", &name) < 0)
+        return FALSE;

     ret = cmdRunConsole(ctl, dom, name);

@@ -2272,7 +2275,7 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd)
 {
     int func_ret = FALSE;
     int ret;
-    int cell, cell_given;
+    int cell = -1, cell_given;
     unsigned long long memory;
     xmlNodePtr *nodes = NULL;
     unsigned long nodes_cnt;
@@ -2406,7 +2409,8 @@ cmdMaxvcpus(vshControl *ctl, const vshCmd *cmd)
     const char *type = NULL;
     int vcpus;

-    vshCommandOptString(cmd, "type", &type);
+    if (vshCommandOptString(cmd, "type", &type) < 0)
+        return FALSE;

     if (!vshConnectionUsability(ctl, ctl->conn))
         return FALSE;
@@ -2851,7 +2855,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         return FALSE;

-    vshCommandOptInt(cmd, "count", &count);
+    if (vshCommandOptInt(cmd, "count", &count) < 0)
+        return FALSE;

     if (!flags) {
         if (virDomainSetVcpus(dom, count) != 0) {
@@ -2978,7 +2983,7 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
         return FALSE;

     if (vshCommandOptInt(cmd, "kilobytes", &kilobytes) < 0) {
-        vshError(ctl, "%s", _("memory sizehas to be a number"));
+        vshError(ctl, "%s", _("memory size has to be a number"));
         return FALSE;
     }

@@ -3040,7 +3045,8 @@ static int
 cmdMemtune(vshControl * ctl, const vshCmd * cmd)
 {
     virDomainPtr dom;
-    long long hard_limit = 0, soft_limit = 0, swap_hard_limit = 0,
min_guarantee = 0;
+    long long hard_limit = 0, soft_limit = 0, swap_hard_limit = 0;
+    long long min_guarantee = 0;
     int nparams = 0;
     unsigned int i = 0;
     virMemoryParameterPtr params = NULL, temp = NULL;
@@ -3052,19 +3058,24 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         return FALSE;

-    vshCommandOptLongLong(cmd, "hard-limit", (unsigned long long*)
&hard_limit);
+    if (vshCommandOptLongLong(cmd, "hard-limit", &hard_limit) < 0 ||
+        vshCommandOptLongLong(cmd, "soft-limit", &soft_limit) < 0 ||
+        vshCommandOptLongLong(cmd, "swap-hard-limit", &swap_hard_limit)
< 0 ||
+        vshCommandOptLongLong(cmd, "min-guarantee", &min_guarantee) < 0) {
+        vshError(ctl, "%s",
+                 _("Unable to parse integer parameter"));
+        goto cleanup;
+    }
+
     if (hard_limit)
         nparams++;

-    vshCommandOptLongLong(cmd, "soft-limit", (unsigned long long*)
&soft_limit);
     if (soft_limit)
         nparams++;

-    vshCommandOptLongLong(cmd, "swap-hard-limit", (unsigned long long*)
&swap_hard_limit);
     if (swap_hard_limit)
         nparams++;

-    vshCommandOptLongLong(cmd, "min-guarantee", (unsigned long long*)
&min_guarantee);
     if (min_guarantee)
         nparams++;

@@ -3317,8 +3328,9 @@ cmdDomXMLFromNative(vshControl *ctl, const vshCmd
*cmd)
     if (!vshConnectionUsability(ctl, ctl->conn))
         return FALSE;

-    vshCommandOptString(cmd, "format", &format);
-    vshCommandOptString(cmd, "config", &configFile);
+    if (vshCommandOptString(cmd, "format", &format) < 0 ||
+        vshCommandOptString(cmd, "config", &configFile) < 0)
+        return FALSE;

     if (virFileReadAll(configFile, 1024*1024, &configData) < 0)
         return FALSE;
@@ -3362,8 +3374,9 @@ cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd)
     if (!vshConnectionUsability(ctl, ctl->conn))
         return FALSE;

-    vshCommandOptString(cmd, "format", &format);
-    vshCommandOptString(cmd, "xml", &xmlFile);
+    if (vshCommandOptString(cmd, "format", &format) < 0
+        || vshCommandOptString(cmd, "xml", &xmlFile) < 0)
+        return FALSE;

     if (virFileReadAll(xmlFile, 1024*1024, &xmlData) < 0)
         return FALSE;
@@ -3540,13 +3553,11 @@ doMigrate (void *opaque)
     if (!(dom = vshCommandOptDomain (ctl, cmd, NULL)))
         goto out;

-    if (vshCommandOptString (cmd, "desturi", &desturi) <= 0)
+    if (vshCommandOptString(cmd, "desturi", &desturi) <= 0 ||
+        vshCommandOptString(cmd, "migrateuri", &migrateuri) < 0 ||
+        vshCommandOptString(cmd, "dname", &dname) < 0)
         goto out;

-    vshCommandOptString (cmd, "migrateuri", &migrateuri);
-
-    vshCommandOptString (cmd, "dname", &dname);
-
     if (vshCommandOptBool (cmd, "live"))
         flags |= VIR_MIGRATE_LIVE;
     if (vshCommandOptBool (cmd, "p2p"))
@@ -3794,8 +3805,8 @@ cmdMigrateSetMaxDowntime(vshControl *ctl, const
vshCmd *cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         return FALSE;

-    vshCommandOptLongLong(cmd, "downtime", (unsigned long long*)
&downtime);
-    if (downtime < 1) {
+    if (vshCommandOptLongLong(cmd, "downtime", &downtime) < 0 ||
+        downtime < 1) {
         vshError(ctl, "%s", _("migrate: Invalid downtime"));
         goto done;
     }
@@ -5343,12 +5354,13 @@ static int buildPoolXML(const vshCmd *cmd, const
char **retname, char **xml) {
     if (vshCommandOptString(cmd, "type", &type) <= 0)
         goto cleanup;

-    vshCommandOptString(cmd, "source-host", &srcHost);
-    vshCommandOptString(cmd, "source-path", &srcPath);
-    vshCommandOptString(cmd, "source-dev", &srcDev);
-    vshCommandOptString(cmd, "source-name", &srcName);
-    vshCommandOptString(cmd, "source-format", &srcFormat);
-    vshCommandOptString(cmd, "target", &target);
+    if (vshCommandOptString(cmd, "source-host", &srcHost) < 0 ||
+        vshCommandOptString(cmd, "source-path", &srcPath) < 0 ||
+        vshCommandOptString(cmd, "source-dev", &srcDev) < 0 ||
+        vshCommandOptString(cmd, "source-name", &srcName) < 0 ||
+        vshCommandOptString(cmd, "source-format", &srcFormat) < 0 ||
+        vshCommandOptString(cmd, "target", &target) < 0)
+        goto cleanup;

     virBufferVSprintf(&buf, "<pool type='%s'>\n", type);
     virBufferVSprintf(&buf, "  <name>%s</name>\n", name);
@@ -6150,18 +6162,23 @@ cmdPoolDiscoverSourcesAs(vshControl * ctl, const
vshCmd * cmd ATTRIBUTE_UNUSED)
     char *srcList;
     const char *initiator = NULL;

-    if (vshCommandOptString(cmd, "type", &type) <= 0)
+    if (vshCommandOptString(cmd, "type", &type) <= 0 ||
+        vshCommandOptString(cmd, "host", &host) < 0 ||
+        vshCommandOptString(cmd, "initiator", &initiator) < 0)
         return FALSE;
-    vshCommandOptString(cmd, "host", &host);
-    vshCommandOptString(cmd, "initiator", &initiator);

     if (!vshConnectionUsability(ctl, ctl->conn))
         return FALSE;

     if (host) {
         const char *port = NULL;
-        vshCommandOptString(cmd, "port", &port);
         virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+        if (vshCommandOptString(cmd, "port", &port) < 0) {
+            vshError(ctl, "%s", _("missing argument"));
+            virBufferFreeAndReset(&buf);
+            return FALSE;
+        }
         virBufferAddLit(&buf, "<source>\n");
         virBufferVSprintf(&buf, "  <host name='%s'", host);
         if (port)
@@ -6219,7 +6236,8 @@ cmdPoolDiscoverSources(vshControl * ctl, const
vshCmd * cmd ATTRIBUTE_UNUSED)
     if (vshCommandOptString(cmd, "type", &type) <= 0)
         return FALSE;

-    vshCommandOptString(cmd, "srcSpec", &srcSpecFile);
+    if (vshCommandOptString(cmd, "srcSpec", &srcSpecFile) < 0)
+        return FALSE;

     if (!vshConnectionUsability(ctl, ctl->conn))
         return FALSE;
@@ -6485,9 +6503,13 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd)
         (cmdVolSize(allocationStr, &allocation) < 0))
         vshError(ctl, _("Malformed size %s"), allocationStr);

-    vshCommandOptString(cmd, "format", &format);
-    vshCommandOptString(cmd, "backing-vol", &snapshotStrVol);
-    vshCommandOptString(cmd, "backing-vol-format", &snapshotStrFormat);
+    if (vshCommandOptString(cmd, "format", &format) < 0 ||
+        vshCommandOptString(cmd, "backing-vol", &snapshotStrVol) < 0 ||
+        vshCommandOptString(cmd, "backing-vol-format",
+                            &snapshotStrFormat) < 0) {
+        vshError(ctl, "%s", _("missing argument"));
+    }
+

     virBufferAddLit(&buf, "<volume>\n");
     virBufferVSprintf(&buf, "  <name>%s</name>\n", name);
@@ -8686,11 +8708,12 @@ cmdAttachInterface(vshControl *ctl, const vshCmd
*cmd)
     if (vshCommandOptString(cmd, "type", &type) <= 0)
         goto cleanup;

-    vshCommandOptString(cmd, "source", &source);
-    vshCommandOptString(cmd, "target", &target);
-    vshCommandOptString(cmd, "mac", &mac);
-    vshCommandOptString(cmd, "script", &script);
-    vshCommandOptString(cmd, "model", &model);
+    if (vshCommandOptString(cmd, "source", &source) < 0 ||
+        vshCommandOptString(cmd, "target", &target) < 0 ||
+        vshCommandOptString(cmd, "mac", &mac) < 0 ||
+        vshCommandOptString(cmd, "script", &script) < 0 ||
+        vshCommandOptString(cmd, "model", &model) < 0)
+        goto cleanup;

     /* check interface type */
     if (STREQ(type, "network")) {
@@ -8796,7 +8819,8 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
     if (vshCommandOptString(cmd, "type", &type) <= 0)
         goto cleanup;

-    vshCommandOptString(cmd, "mac", &mac);
+    if (vshCommandOptString(cmd, "mac", &mac) < 0)
+        goto cleanup;

     doc = virDomainGetXMLDesc(dom, 0);
     if (!doc)
@@ -8941,11 +8965,13 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
     if (vshCommandOptString(cmd, "target", &target) <= 0)
         goto cleanup;

-    vshCommandOptString(cmd, "driver", &driver);
-    vshCommandOptString(cmd, "subdriver", &subdriver);
-    vshCommandOptString(cmd, "type", &type);
-    vshCommandOptString(cmd, "mode", &mode);
-    vshCommandOptString(cmd, "sourcetype", &stype);
+    if (vshCommandOptString(cmd, "driver", &driver) < 0 ||
+        vshCommandOptString(cmd, "subdriver", &subdriver) < 0 ||
+        vshCommandOptString(cmd, "type", &type) < 0 ||
+        vshCommandOptString(cmd, "mode", &mode) < 0 ||
+        vshCommandOptString(cmd, "sourcetype", &stype) < 0) {
+        goto cleanup;
+    }

     if (!stype) {
         if (driver && (STREQ(driver, "file") || STREQ(driver, "tap")))
@@ -10752,7 +10778,7 @@ vshCommandOptInt(const vshCmd *cmd, const char
*name, int *value)
     if ((arg != NULL) && (arg->data != NULL)) {
         num = strtol(arg->data, &end_p, 10);
         ret = -1;
-        if ((arg->data != end_p) && (*end_p == 0) && value) {
+        if ((arg->data != end_p) && (*end_p == 0)) {
             *value = num;
             ret = 1;
         }
@@ -10775,7 +10801,7 @@ vshCommandOptUL(const vshCmd *cmd, const char
*name, unsigned long *value)
     if ((arg != NULL) && (arg->data != NULL)) {
         num = strtoul(arg->data, &end_p, 10);
         ret = -1;
-        if ((arg->data != end_p) && (*end_p == 0) && value) {
+        if ((arg->data != end_p) && (*end_p == 0)) {
             *value = num;
             ret = 1;
         }
@@ -10801,7 +10827,7 @@ vshCommandOptString(const vshCmd *cmd, const
char *name, const char **value)
                 ret = 1;
             }
         } else if (arg->def && ((arg->def->flag) & VSH_OFLAG_REQ)) {
-                vshError(NULL, _("Missing required option '%s'"), name);
+            vshError(NULL, _("Missing required option '%s'"), name);
         }
     }

@@ -10814,7 +10840,7 @@ vshCommandOptString(const vshCmd *cmd, const
char *name, const char **value)
  */
 static int
 vshCommandOptLongLong(const vshCmd *cmd, const char *name,
-                      unsigned long long *value)
+                      long long *value)
 {
     vshCmdOpt *arg = vshCommandOpt(cmd, name);
     int ret = 0;
@@ -10824,7 +10850,7 @@ vshCommandOptLongLong(const vshCmd *cmd, const
char *name,
     if ((arg != NULL) && (arg->data != NULL)) {
         num = strtoll(arg->data, &end_p, 10);
         ret = -1;
-        if ((arg->data != end_p) && (*end_p == 0) && value) {
+        if ((arg->data != end_p) && (*end_p == 0)) {
             *value = num;
             ret = 1;
         }


-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]