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