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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list