On 2013年01月22日 02:07, Peter Krempa wrote:

buffer won't be NULL in any case when reaching the check and the first block of the if statement was lacking parentheses --- tools/virsh-snapshot.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index e4745d6..f76c40e 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -204,26 +204,17 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;

- dom = vshCommandOptDomain(ctl, cmd, NULL); - if (dom == NULL) + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup;

- if (vshCommandOptString(cmd, "xmlfile",&from)<= 0) + if (vshCommandOptString(cmd, "xmlfile",&from)<= 0) { buffer = vshStrdup(ctl, "<domainsnapshot/>"); - else { + } else { if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) { - /* we have to report the error here because during cleanup - * we'll run through virDomainFree(), which loses the - * last error - */ - vshReportError(ctl); + vshSaveLibvirtError(); goto cleanup; } } - if (buffer == NULL) { - vshError(ctl, "%s", _("Out of memory")); - goto cleanup; - }

ret = vshSnapshotCreate(ctl, dom, buffer, flags, from);

ACK.

On 2013年01月22日 02:07, Peter Krempa wrote:

--- tools/virsh-volume.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index e17d946..466188c 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -464,7 +464,7 @@ cleanup: }

static xmlChar * -makeCloneXML(const char *origxml, const char *newname) +vshMakeCloneXML(const char *origxml, const char *newname) {

xmlDocPtr doc = NULL; @@ -555,7 +555,7 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd) if (!origxml) goto cleanup;

- newxml = makeCloneXML(origxml, name); + newxml = vshMakeCloneXML(origxml, name); if (!newxml) { vshPrint(ctl, "%s", _("Failed to allocate XML buffer")); goto cleanup;

ACK

On 2013年01月22日 02:07, Peter Krempa wrote:

--- tools/virsh-pool.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index cb9866f..bc400a1 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -226,7 +226,7 @@ static const vshCmdOptDef opts_pool_X_as[] = { {.name = NULL} };

-static int buildPoolXML(const vshCmd *cmd, const char **retname, char **xml) { +static int vshBuildPoolXML(const vshCmd *cmd, const char **retname, char **xml) {

Deserved to changed the style meanwhile: static int vshBuildPoolXML (...)

const char *name = NULL, *type = NULL, *srcHost = NULL, *srcPath = NULL, *srcDev = NULL, *srcName = NULL, *srcFormat = NULL, *target = NULL; @@ -303,7 +303,7 @@ cmdPoolCreateAs(vshControl *ctl, const vshCmd *cmd) char *xml; bool printXML = vshCommandOptBool(cmd, "print-xml");

- if (!buildPoolXML(cmd,&name,&xml)) + if (!vshBuildPoolXML(cmd,&name,&xml)) return false;

if (printXML) { @@ -387,7 +387,7 @@ cmdPoolDefineAs(vshControl *ctl, const vshCmd *cmd) char *xml; bool printXML = vshCommandOptBool(cmd, "print-xml");

- if (!buildPoolXML(cmd,&name,&xml)) + if (!vshBuildPoolXML(cmd,&name,&xml)) return false;

if (printXML) {

ACK with the style change.

On 2013年01月22日 02:07, Peter Krempa wrote:

This patch adds a helper function with similar semantics to vshCommandOptString that requests a string argument, but does some error reporting without the need to do it in the functions themselves.

The error reporting also provides information about the parameter whose retrieval failed. --- tools/virsh.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.h | 4 ++++ 2 files changed, 55 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c index 908c6a1..1a3cab0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1417,6 +1417,57 @@ vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) }

/** + * vshCommandOptStringReq: Get a required string argumment

Trivial, but we usually describe what the function does at [1].

+ * @ctl virsh control structure

And have a ":" after @foo.

+ * @cmd command structure + * @name option name + * @value result (updated to NULL or the actual value)

s/actual value/option argument/,

+ *

[1]. Right here.

+ * Returns option as string. Prints error message if needed

I think you mean option argument here.

+ * Return value: + * 0 if option is correct (available or not required) + * -1 and error message printed on error

Not to be captious. But how about rewording above like: Returns 0 on success or the option is not present and not requirent, *value is set to the option argument if success; Or -1 on failure, with error message printed.

+ */ +int +vshCommandOptStringReq(vshControl *ctl, + const vshCmd *cmd, + const char *name, + const char **value) +{ + vshCmdOpt *arg; + int ret; + const char *error = NULL; + + /* clear out the value */ + *value = NULL; + + ret = vshCommandOpt(cmd, name,&arg); + /* option is not required and not present */ + if (ret == 0) + return 0; + /* this should not be propagated here, just to be sure */ + if (ret == -1) + error = N_("Mandatory option not present"); + + if (ret == -2) + error = N_("Programming error: Invalid option name"); + + if (!arg->data) + error = N_("Programming error: Requested option is a boolean"); + + if (!*arg->data&& !(arg->def->flags& VSH_OFLAG_EMPTY_OK)) + error = N_("Option argument is empty"); + + if (error) { + vshError(ctl, _("Failed to get option '%s': %s"), name, _(error)); + return -1; + } + + *value = arg->data; + return 0; +} + +/** * vshCommandOptLongLong: * @cmd command reference * @name option name diff --git a/tools/virsh.h b/tools/virsh.h index ab7161f..fec9785 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -275,6 +275,10 @@ int vshCommandOptUL(const vshCmd *cmd, const char *name, int vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK; +int vshCommandOptStringReq(vshControl *ctl, const vshCmd *cmd, + const char *name, const char **value) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; int vshCommandOptLongLong(const vshCmd *cmd, const char *name, long long *value) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;

ACK with the small nits fixed.

On 2013年02月04日 18:47, Peter Krempa wrote:

On 01/31/13 06:18, Osier Yang wrote:

...

On 2013年01月22日 02:07, Peter Krempa wrote:

...

This patch adds a helper function with similar semantics to vshCommandOptString that requests a string argument, but does some error reporting without the need to do it in the functions themselves.

The error reporting also provides information about the parameter whose retrieval failed. --- tools/virsh.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.h | 4 ++++ 2 files changed, 55 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c index 908c6a1..1a3cab0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1417,6 +1417,57 @@ vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) }

/** + * vshCommandOptStringReq: Get a required string argumment

Trivial, but we usually describe what the function does at [1].

...

+ * @ctl virsh control structure

And have a ":" after @foo.

Not in virsh.c. All surrounding functions don't have the colon in the comment. This is worth cleaning up separately instead of doing it in multiple ways in a single file.

Agreed, a follow up patch will be good then. But others still stands.

...

...

+ * @cmd command structure + * @name option name + * @value result (updated to NULL or the actual value)

s/actual value/option argument/,

...

+ *

[1]. Right here.

Peter

On 2013年02月04日 19:32, Peter Krempa wrote:

On 02/04/13 12:28, Osier Yang wrote:

...

On 2013年02月04日 18:47, Peter Krempa wrote:

...

On 01/31/13 06:18, Osier Yang wrote:

...

On 2013年01月22日 02:07, Peter Krempa wrote:

...

This patch adds a helper function with similar semantics to vshCommandOptString that requests a string argument, but does some error reporting without the need to do it in the functions themselves.

The error reporting also provides information about the parameter whose retrieval failed. --- tools/virsh.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.h | 4 ++++ 2 files changed, 55 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c index 908c6a1..1a3cab0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1417,6 +1417,57 @@ vshCommandOptString(const vshCmd *cmd, const char *name, const char **value) }

/** + * vshCommandOptStringReq: Get a required string argumment

Trivial, but we usually describe what the function does at [1].

...

+ * @ctl virsh control structure

And have a ":" after @foo.

Not in virsh.c. All surrounding functions don't have the colon in the comment. This is worth cleaning up separately instead of doing it in multiple ways in a single file.

Agreed, a follow up patch will be good then. But others still stands.

I changed the comment to: /** + * vshCommandOptStringReq: + * @ctl virsh control structure + * @cmd command structure + * @name option name + * @value result (updated to NULL or the option argument) + * + * Gets a option argument as string. This function reports errors.

I think "The function reports errors" can just be removed. As it doesn't report any error on success.

+ * + * Returns 0 on success or when the option is not present and not + * required, *value is set to the option argument. On error -1 is + * returned and error message printed. + */

Peter

ACK with that.

On 01/31/13 06:22, Osier Yang wrote:

On 2013年01月22日 02:07, Peter Krempa wrote:

...

Rework most of option string retrievals to the new helper. --- tools/virsh-pool.c | 56 +++++++++++++++++++++++++----------------------------- 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index bc400a1..5a7c8dd 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c

[...]

...

@@ -226,26 +226,28 @@ static const vshCmdOptDef opts_pool_X_as[] = { {.name = NULL} };

-static int vshBuildPoolXML(const vshCmd *cmd, const char **retname, char **xml) { - +static int +vshBuildPoolXML(vshControl *ctl, + const vshCmd *cmd, + const char **retname, + char **xml)

Okay, you change the style here. :-)

Yep, part of this hunk belongs to the patch before.

ACK

Thanks. Peter

On 2013年01月22日 02:07, Peter Krempa wrote:

--- tools/virsh-volume.c | 69 +++++++++++++++++++++------------------------------- 1 file changed, 28 insertions(+), 41 deletions(-)

diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 466188c..0575166 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -52,13 +52,12 @@ vshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd, const char *n = NULL, *p = NULL; virCheckFlags(VSH_BYUUID | VSH_BYNAME, NULL);

- if (vshCommandOptString(cmd, optname,&n)<= 0) + if (vshCommandOptStringReq(ctl, cmd, optname,&n)< 0) return NULL;

- if (pooloptname != NULL&& vshCommandOptString(cmd, pooloptname,&p)< 0) { - vshError(ctl, "%s", _("missing option")); + if (pooloptname != NULL&& + vshCommandOptStringReq(ctl, cmd, pooloptname,&p)< 0) return NULL; - }

if (p) pool = vshCommandOptPoolBy(ctl, cmd, pooloptname, name, flags); @@ -182,10 +181,10 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd) VSH_BYNAME))) return false;

- if (vshCommandOptString(cmd, "name",&name)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "name",&name)< 0) goto cleanup;

- if (vshCommandOptString(cmd, "capacity",&capacityStr)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "capacity",&capacityStr)< 0) goto cleanup;

if (vshVolSize(capacityStr,&capacity)< 0) { @@ -199,14 +198,11 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd) goto cleanup; }

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

virBufferAddLit(&buf, "<volume>\n"); virBufferAsprintf(&buf, "<name>%s</name>\n", name); @@ -340,9 +336,9 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd) virStoragePoolPtr pool; virStorageVolPtr vol; const char *from = NULL; - bool ret = true; + bool ret = false; unsigned int flags = 0; - char *buffer; + char *buffer = NULL;

if (vshCommandOptBool(cmd, "prealloc-metadata")) flags |= VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA; @@ -350,29 +346,26 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd) VSH_BYNAME))) return false;

- if (vshCommandOptString(cmd, "file",&from)<= 0) { - virStoragePoolFree(pool); - return false; - } + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) + goto cleanup;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) { - vshReportError(ctl); - virStoragePoolFree(pool); - return false; + vshSaveLibvirtError(); + goto cleanup; }

- vol = virStorageVolCreateXML(pool, buffer, flags); - VIR_FREE(buffer); - virStoragePoolFree(pool); - - if (vol != NULL) { + if ((vol = virStorageVolCreateXML(pool, buffer, flags))) { vshPrint(ctl, _("Vol %s created from %s\n"), virStorageVolGetName(vol), from); virStorageVolFree(vol); + ret = true; } else { vshError(ctl, _("Failed to create vol from %s"), from); - ret = false; } + +cleanup: + VIR_FREE(buffer); + virStoragePoolFree(pool); return ret; }

@@ -429,9 +422,9 @@ cmdVolCreateFrom(vshControl *ctl, const vshCmd *cmd)

if (vshCommandOptBool(cmd, "prealloc-metadata")) flags |= VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA; - if (vshCommandOptString(cmd, "file",&from)<= 0) { + + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) goto cleanup; - }

if (!(inputvol = vshCommandOptVol(ctl, cmd, "vol", "inputpool", NULL))) goto cleanup; @@ -548,7 +541,7 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd) goto cleanup; }

- if (vshCommandOptString(cmd, "newname",&name)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "newname",&name)< 0) goto cleanup;

origxml = virStorageVolGetXMLDesc(origvol, 0); @@ -658,10 +651,8 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) return false; }

- if (vshCommandOptString(cmd, "file",&file)< 0) { - vshError(ctl, _("file must not be empty")); + if (vshCommandOptStringReq(ctl, cmd, "file",&file)< 0) goto cleanup; - }

if ((fd = open(file, O_RDONLY))< 0) { vshError(ctl, _("cannot read %s"), file); @@ -764,10 +755,8 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd) if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool",&name))) return false;

- if (vshCommandOptString(cmd, "file",&file)< 0) { - vshError(ctl, _("file must not be empty")); + if (vshCommandOptStringReq(ctl, cmd, "file",&file)< 0) goto cleanup; - }

if ((fd = open(file, O_WRONLY|O_CREAT|O_EXCL, 0666))< 0) { if (errno != EEXIST || @@ -906,10 +895,8 @@ cmdVolWipe(vshControl *ctl, const vshCmd *cmd) return false; }

- if (vshCommandOptString(cmd, "algorithm",&algorithm_str)< 0) { - vshError(ctl, "%s", _("missing argument")); + if (vshCommandOptStringReq(ctl, cmd, "algorithm",&algorithm_str)< 0) goto out; - }

if (algorithm_str&& (algorithm = virStorageVolWipeAlgorithmTypeFromString(algorithm_str))< 0) { @@ -1072,7 +1059,7 @@ cmdVolResize(vshControl *ctl, const vshCmd *cmd) if (!(vol = vshCommandOptVol(ctl, cmd, "vol", "pool", NULL))) return false;

- if (vshCommandOptString(cmd, "capacity",&capacityStr)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "capacity",&capacityStr)<= 0) goto cleanup; virSkipSpaces(&capacityStr); if (*capacityStr == '-') {

ACK

On 01/31/13 07:50, Osier Yang wrote:

On 2013年01月22日 02:07, Peter Krempa wrote:

...

--- tools/virsh-domain-monitor.c | 44 +++++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 1e8b8cf..7e105fd 100644

[...]

...

@@ -891,7 +886,7 @@ cmdDomblkstat(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) return false;

- if (vshCommandOptString(cmd, "device",&device)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "device",&device)<= 0)

I think you mean < 0 here.

right ....

...

goto cleanup;

rc = virDomainBlockStatsFlags(dom, device, NULL,&nparams, 0);

[...]

...

@@ -1055,8 +1048,9 @@ cmdDomIfstat(vshControl *ctl, const vshCmd *cmd) if (stats.tx_drop>= 0) vshPrint(ctl, "%s tx_drop %lld\n", device, stats.tx_drop);

+cleanup: virDomainFree(dom); - return true; + return ret;

Is "ret" set to true yet if success?

I forgot to put "ret = true;" before the cleanup label.

Thanks. Peter

On 2013年01月22日 02:07, Peter Krempa wrote:

--- tools/virsh-domain.c | 299 ++++++++++++++++++++------------------------------- 1 file changed, 116 insertions(+), 183 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f4b6622..e3c43d4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -74,7 +74,7 @@ vshCommandOptDomainBy(vshControl *ctl, const vshCmd *cmd, if (!vshCmdHasOption(ctl, cmd, optname)) return NULL;

- if (vshCommandOptString(cmd, optname,&n)<= 0) + if (vshCommandOptStringReq(ctl, cmd, optname,&n)< 0) return NULL;

vshDebug(ctl, VSH_ERR_INFO, "%s: found option<%s>: %s\n", @@ -188,43 +188,42 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; const char *from = NULL; char *buffer; - int ret; + int rv; + bool ret = false; unsigned int flags;

if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;

- if (vshCommandOptString(cmd, "file",&from)<= 0) { - virDomainFree(dom); - return false; - } + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) + goto cleanup;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) { vshReportError(ctl); - virDomainFree(dom); - return false; + goto cleanup; }

if (vshCommandOptBool(cmd, "config")) { flags = VIR_DOMAIN_AFFECT_CONFIG; if (virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_AFFECT_LIVE; - ret = virDomainAttachDeviceFlags(dom, buffer, flags); + rv = virDomainAttachDeviceFlags(dom, buffer, flags); } else { - ret = virDomainAttachDevice(dom, buffer); + rv = virDomainAttachDevice(dom, buffer); } VIR_FREE(buffer);

- if (ret< 0) { + if (rv< 0) { vshError(ctl, _("Failed to attach device from %s"), from); - virDomainFree(dom); - return false; - } else { - vshPrint(ctl, "%s", _("Device attached successfully\n")); + goto cleanup; }

+ vshPrint(ctl, "%s", _("Device attached successfully\n")); + ret = true; + +cleanup: virDomainFree(dom); - return true; + return ret; }

/* @@ -486,28 +485,18 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup;

- if (vshCommandOptString(cmd, "source",&source)<= 0) - goto cleanup; - /* Allow empty string as a placeholder that implies no source, for - * use in adding a cdrom drive with no disk. */ - if (!*source) - source = NULL; - - if (vshCommandOptString(cmd, "target",&target)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "source",&source)< 0 || + vshCommandOptStringReq(ctl, cmd, "target",&target)< 0 || + vshCommandOptStringReq(ctl, cmd, "driver",&driver)< 0 || + vshCommandOptStringReq(ctl, cmd, "subdriver",&subdriver)< 0 || + vshCommandOptStringReq(ctl, cmd, "type",&type)< 0 || + vshCommandOptStringReq(ctl, cmd, "mode",&mode)< 0 || + vshCommandOptStringReq(ctl, cmd, "cache",&cache)< 0 || + vshCommandOptStringReq(ctl, cmd, "serial",&serial)< 0 || + vshCommandOptStringReq(ctl, cmd, "address",&straddr)< 0 || + vshCommandOptStringReq(ctl, cmd, "sourcetype",&stype)< 0) goto cleanup;

- if (vshCommandOptString(cmd, "driver",&driver)< 0 || - vshCommandOptString(cmd, "subdriver",&subdriver)< 0 || - vshCommandOptString(cmd, "type",&type)< 0 || - vshCommandOptString(cmd, "mode",&mode)< 0 || - vshCommandOptString(cmd, "cache",&cache)< 0 || - vshCommandOptString(cmd, "serial",&serial)< 0 || - vshCommandOptString(cmd, "address",&straddr)< 0 || - vshCommandOptString(cmd, "sourcetype",&stype)< 0) { - vshError(ctl, "%s", _("missing option")); - goto cleanup; - } - if (!stype) { if (driver&& (STREQ(driver, "file") || STREQ(driver, "tap"))) { isFile = true; @@ -766,20 +755,16 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup;

- if (vshCommandOptString(cmd, "type",&type)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "type",&type)< 0 || + vshCommandOptStringReq(ctl, cmd, "source",&source)< 0 || + vshCommandOptStringReq(ctl, cmd, "target",&target)< 0 || + vshCommandOptStringReq(ctl, cmd, "mac",&mac)< 0 || + vshCommandOptStringReq(ctl, cmd, "script",&script)< 0 || + vshCommandOptStringReq(ctl, cmd, "model",&model)< 0 || + vshCommandOptStringReq(ctl, cmd, "inbound",&inboundStr)< 0 || + vshCommandOptStringReq(ctl, cmd, "outbound",&outboundStr)< 0) goto cleanup;

- 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 || - vshCommandOptString(cmd, "inbound",&inboundStr)< 0 || - vshCommandOptString(cmd, "outbound",&outboundStr)< 0) { - vshError(ctl, "missing argument"); - goto cleanup; - } - /* check interface type */ if (STREQ(type, "network")) { typ = 1; @@ -1070,7 +1055,7 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) goto cleanup;

- if (vshCommandOptString(cmd, "device",&disk)< 0) + if (vshCommandOptStringReq(ctl, cmd, "device",&disk)< 0) goto cleanup;

if ((rv = vshCommandOptULongLong(cmd, "total-bytes-sec",&value))< 0) { @@ -1349,7 +1334,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) goto cleanup;

- if (vshCommandOptString(cmd, "path",&path)< 0) + if (vshCommandOptStringReq(ctl, cmd, "path",&path)< 0) goto cleanup;

if (vshCommandOptUL(cmd, "bandwidth",&bandwidth)< 0) { @@ -1372,7 +1357,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, ret = virDomainBlockJobSetSpeed(dom, path, bandwidth, 0); break; case VSH_CMD_BLOCK_JOB_PULL: - if (vshCommandOptString(cmd, "base",&base)< 0) + if (vshCommandOptStringReq(ctl, cmd, "base",&base)< 0) goto cleanup; if (base) ret = virDomainBlockRebase(dom, path, base, bandwidth, 0); @@ -1380,8 +1365,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, ret = virDomainBlockPull(dom, path, bandwidth, 0); break; case VSH_CMD_BLOCK_JOB_COMMIT: - if (vshCommandOptString(cmd, "base",&base)< 0 || - vshCommandOptString(cmd, "top",&top)< 0) + if (vshCommandOptStringReq(ctl, cmd, "base",&base)< 0 || + vshCommandOptStringReq(ctl, cmd, "top",&top)< 0) goto cleanup; if (vshCommandOptBool(cmd, "shallow")) flags |= VIR_DOMAIN_BLOCK_COMMIT_SHALLOW; @@ -1397,7 +1382,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; if (vshCommandOptBool(cmd, "raw")) flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; - if (vshCommandOptString(cmd, "dest",&base)< 0) + if (vshCommandOptStringReq(ctl, cmd, "dest",&base)< 0) goto cleanup; ret = virDomainBlockRebase(dom, path, base, bandwidth, flags); } @@ -1541,7 +1526,7 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) } timeout *= 1000; } - if (vshCommandOptString(cmd, "path",&path)< 0) + if (vshCommandOptStringReq(ctl, cmd, "path",&path)< 0) return false; if (vshCommandOptBool(cmd, "async")) abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; @@ -1741,7 +1726,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) } timeout *= 1000; } - if (vshCommandOptString(cmd, "path",&path)< 0) + if (vshCommandOptStringReq(ctl, cmd, "path",&path)< 0) return false; if (vshCommandOptBool(cmd, "async")) abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; @@ -2017,7 +2002,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) } timeout *= 1000; } - if (vshCommandOptString(cmd, "path",&path)< 0) + if (vshCommandOptStringReq(ctl, cmd, "path",&path)< 0) return false; if (vshCommandOptBool(cmd, "async")) abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC; @@ -2138,10 +2123,8 @@ cmdBlockResize(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0; int ret = false;

- if (vshCommandOptString(cmd, "path", (const char **)&path)< 0) { - vshError(ctl, "%s", _("Path must not be empty")); + if (vshCommandOptStringReq(ctl, cmd, "path", (const char **)&path)< 0) return false; - }

if (vshCommandOptScaledInt(cmd, "size",&size, 1024, ULLONG_MAX)< 0) { vshError(ctl, "%s", _("Unable to parse integer")); @@ -2250,10 +2233,8 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;

- if (vshCommandOptString(cmd, "devname",&name)< 0) { - vshError(ctl, "%s", _("Invalid devname")); + if (vshCommandOptStringReq(ctl, cmd, "devname",&name)< 0) goto cleanup; - }

if (force) flags |= VIR_DOMAIN_CONSOLE_FORCE; @@ -2329,10 +2310,8 @@ cmdDomIfSetLink(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;

- if (vshCommandOptString(cmd, "interface",&iface)<= 0) - goto cleanup; - - if (vshCommandOptString(cmd, "state",&state)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "interface",&iface)< 0 || + vshCommandOptStringReq(ctl, cmd, "state",&state)< 0) goto cleanup;

config = vshCommandOptBool(cmd, "config"); @@ -2542,14 +2521,12 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) return false;

- if (vshCommandOptString(cmd, "interface",&device)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "interface",&device)< 0) goto cleanup;

- if (vshCommandOptString(cmd, "inbound",&inboundStr)< 0 || - vshCommandOptString(cmd, "outbound",&outboundStr)< 0) { - vshError(ctl, "missing argument"); + if (vshCommandOptStringReq(ctl, cmd, "inbound",&inboundStr)< 0 || + vshCommandOptStringReq(ctl,cmd, "outbound",&outboundStr)< 0) goto cleanup; - }

memset(&inbound, 0, sizeof(inbound)); memset(&outbound, 0, sizeof(outbound)); @@ -2746,10 +2723,8 @@ cmdDomPMSuspend(vshControl *ctl, const vshCmd *cmd) goto cleanup; }

- if (vshCommandOptString(cmd, "target",&target)< 0) { - vshError(ctl, _("Invalid target argument")); + if (vshCommandOptStringReq(ctl, cmd, "target",&target)< 0) goto cleanup; - }

if (STREQ(target, "mem")) suspendTarget = VIR_NODE_SUSPEND_TARGET_MEM; @@ -3374,7 +3349,7 @@ doSave(void *opaque) if (pthread_sigmask(SIG_BLOCK,&sigmask,&oldsigmask)< 0) goto out_sig;

- if (vshCommandOptString(cmd, "file",&to)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&to)< 0) goto out;

if (vshCommandOptBool(cmd, "bypass-cache")) @@ -3384,10 +3359,8 @@ doSave(void *opaque) if (vshCommandOptBool(cmd, "paused")) flags |= VIR_DOMAIN_SAVE_PAUSED;

- if (vshCommandOptString(cmd, "xml",&xmlfile)< 0) { - vshError(ctl, "%s", _("malformed xml argument")); + if (vshCommandOptStringReq(ctl, cmd, "xml",&xmlfile)< 0) goto out; - }

if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) goto out; @@ -3523,7 +3496,7 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) return false;

- if (vshCommandOptString(cmd, "file",&to)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&to)< 0) goto cleanup;

if (vshCommandOptBool(cmd, "verbose")) @@ -3590,7 +3563,7 @@ cmdSaveImageDumpxml(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "security-info")) flags |= VIR_DOMAIN_XML_SECURE;

- if (vshCommandOptString(cmd, "file",&file)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&file)< 0) return false;

xml = virDomainSaveImageGetXMLDesc(ctl->conn, file, flags); @@ -3652,13 +3625,11 @@ cmdSaveImageDefine(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "paused")) flags |= VIR_DOMAIN_SAVE_PAUSED;

- if (vshCommandOptString(cmd, "file",&file)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&file)< 0) return false;

- if (vshCommandOptString(cmd, "xml",&xmlfile)<= 0) { - vshError(ctl, "%s", _("malformed or missing xml argument")); + if (vshCommandOptStringReq(ctl, cmd, "xml",&xmlfile)<= 0)

Again, think you mean "< 0"

return false; - }

if (virFileReadAll(xmlfile, 8192,&xml)< 0) goto cleanup; @@ -3726,7 +3697,7 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) return false; }

- if (vshCommandOptString(cmd, "file",&file)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&file)< 0) return false;

#define EDIT_GET_XML \ @@ -4213,7 +4184,7 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) const char *xmlfile = NULL; char *xml = NULL;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (vshCommandOptBool(cmd, "bypass-cache")) @@ -4223,10 +4194,8 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "paused")) flags |= VIR_DOMAIN_SAVE_PAUSED;

- if (vshCommandOptString(cmd, "xml",&xmlfile)< 0) { - vshError(ctl, "%s", _("malformed xml argument")); + if (vshCommandOptStringReq(ctl, cmd, "xml",&xmlfile)< 0) return false; - }

if (xmlfile&& virFileReadAll(xmlfile, 8192,&xml)< 0) @@ -4318,7 +4287,7 @@ doDump(void *opaque) if (pthread_sigmask(SIG_BLOCK,&sigmask,&oldsigmask)< 0) goto out_sig;

- if (vshCommandOptString(cmd, "file",&to)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&to)< 0) goto out;

if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) @@ -4364,7 +4333,7 @@ cmdDump(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) return false;

- if (vshCommandOptString(cmd, "file",&to)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&to)< 0) return false;

if (vshCommandOptBool(cmd, "verbose")) @@ -4474,10 +4443,8 @@ cmdScreenshot(vshControl *ctl, const vshCmd *cmd) bool generated = false; char *mime = NULL;

- if (vshCommandOptString(cmd, "file", (const char **)&file)< 0) { - vshError(ctl, "%s", _("file must not be empty")); + if (vshCommandOptStringReq(ctl, cmd, "file", (const char **)&file)< 0) return false; - }

if (vshCommandOptUInt(cmd, "screen",&screen)< 0) { vshError(ctl, "%s", _("invalid screen ID")); @@ -4615,10 +4582,8 @@ cmdShutdown(vshControl *ctl, const vshCmd *cmd) int rv; char **modes = NULL, **tmp;

- if (vshCommandOptString(cmd, "mode",&mode)< 0) { - vshError(ctl, "%s", _("Invalid type")); + if (vshCommandOptStringReq(ctl, cmd, "mode",&mode)< 0) return false; - }

if (mode&& !(modes = virStringSplit(mode, ",", 0))) { vshError(ctl, "%s", _("Cannot parse mode string")); @@ -4699,10 +4664,8 @@ cmdReboot(vshControl *ctl, const vshCmd *cmd) int flags = 0; char **modes = NULL, **tmp;

- if (vshCommandOptString(cmd, "mode",&mode)< 0) { - vshError(ctl, "%s", _("Invalid type")); + if (vshCommandOptStringReq(ctl, cmd, "mode",&mode)< 0) return false; - }

if (mode&& !(modes = virStringSplit(mode, ",", 0))) { vshError(ctl, "%s", _("Cannot parse mode string")); @@ -4926,14 +4889,12 @@ cmdMaxvcpus(vshControl *ctl, const vshCmd *cmd) const char *type = NULL; int vcpus;

- if (vshCommandOptString(cmd, "type",&type)< 0) { - vshError(ctl, "%s", _("Invalid type")); + if (vshCommandOptStringReq(ctl, cmd, "type",&type)< 0) return false; - }

- vcpus = virConnectGetMaxVcpus(ctl->conn, type); - if (vcpus< 0) + if ((vcpus = virConnectGetMaxVcpus(ctl->conn, type))< 0) return false; + vshPrint(ctl, "%d\n", vcpus);

return true; @@ -5385,14 +5346,12 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) flags = -1; }

- if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + if (vshCommandOptStringReq(ctl, cmd, "cpulist",&cpulist)< 0) return false;

- if (vshCommandOptString(cmd, "cpulist",&cpulist)< 0) { - vshError(ctl, "%s", _("vcpupin: Missing cpulist.")); - virDomainFree(dom); + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - } + query = !cpulist;

/* In query mode, "vcpu" is optional */ @@ -5623,8 +5582,7 @@ cmdEmulatorPin(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;

- if (vshCommandOptString(cmd, "cpulist",&cpulist)< 0) { - vshError(ctl, "%s", _("emulatorpin: Missing cpulist.")); + if (vshCommandOptStringReq(ctl, cmd, "cpulist",&cpulist)< 0) { virDomainFree(dom); return false; } @@ -5896,14 +5854,11 @@ cmdCPUCompare(vshControl *ctl, const vshCmd *cmd) xmlBufferPtr xml_buf = NULL; xmlNodePtr node;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

- if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) { - vshError(ctl, _("Failed to read file '%s' to compare"), - from); + if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) return false; - }

/* try to extract the CPU element from as it would appear in a domain XML*/ if (!(xml = virXMLParseStringCtxt(buffer, from,&ctxt))) @@ -6000,7 +5955,7 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) virBuffer buf = VIR_BUFFER_INITIALIZER; int i;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) @@ -6287,7 +6242,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd) #endif unsigned int flags = VIR_DOMAIN_NONE;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) @@ -6342,7 +6297,7 @@ cmdDefine(vshControl *ctl, const vshCmd *cmd) bool ret = true; char *buffer;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) @@ -6813,15 +6768,11 @@ cmdSendProcessSignal(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;

- if (vshCommandOptString(cmd, "pid",&pidstr)<= 0) { - vshError(ctl, "%s", _("missing argument")); - return false; - } + if (vshCommandOptStringReq(ctl, cmd, "pid",&pidstr)< 0) + goto cleanup;

- if (vshCommandOptString(cmd, "signame",&signame)<= 0) { - vshError(ctl, "%s", _("missing argument")); - return false; - } + if (vshCommandOptStringReq(ctl, cmd, "signame",&signame)< 0) + goto cleanup;

if (virStrToLong_ll(pidstr, NULL, 10,&pid_value)< 0) { vshError(ctl, _("malformed PID value: %s"), pidstr); @@ -7334,19 +7285,17 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;

- if (vshCommandOptString(cmd, "nodeset",&nodeset)< 0) { - vshError(ctl, "%s", _("Unable to parse nodeset.")); + if (vshCommandOptStringReq(ctl, cmd, "nodeset",&nodeset)< 0) goto cleanup; - } + if (nodeset&& virTypedParamsAddString(¶ms,&nparams,&maxparams, VIR_DOMAIN_NUMA_NODESET, nodeset)< 0) goto save_error;

- if (vshCommandOptString(cmd, "mode",&mode)< 0) { - vshError(ctl, "%s", _("Unable to parse mode.")); + if (vshCommandOptStringReq(ctl, cmd, "mode",&mode)< 0) goto cleanup; - } + if (mode) { int m; /* Accept string or integer, in case server understands newer @@ -7865,8 +7814,8 @@ cmdDomXMLFromNative(vshControl *ctl, const vshCmd *cmd) char *xmlData; unsigned int flags = 0;

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

if (virFileReadAll(configFile, 1024*1024,&configData)< 0) @@ -7917,8 +7866,8 @@ cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd) char *xmlData; unsigned int flags = 0;

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

if (virFileReadAll(xmlFile, 1024*1024,&xmlData)< 0) @@ -8179,17 +8128,11 @@ doMigrate(void *opaque) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto out;

- if (vshCommandOptString(cmd, "desturi",&desturi)<= 0 || - vshCommandOptString(cmd, "migrateuri",&migrateuri)< 0 || - vshCommandOptString(cmd, "dname",&dname)< 0) { - vshError(ctl, "%s", _("missing argument")); - goto out; - } - - if (vshCommandOptString(cmd, "xml",&xmlfile)< 0) { - vshError(ctl, "%s", _("malformed xml argument")); + if (vshCommandOptStringReq(ctl, cmd, "desturi",&desturi)< 0 || + vshCommandOptStringReq(ctl, cmd, "migrateuri",&migrateuri)< 0 || + vshCommandOptStringReq(ctl, cmd, "dname",&dname)< 0 || + vshCommandOptStringReq(ctl, cmd, "xml",&xmlfile)< 0) goto out; - }

if (vshCommandOptBool(cmd, "live")) flags |= VIR_MIGRATE_LIVE; @@ -9115,7 +9058,7 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) goto cleanup;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) { @@ -9189,22 +9132,19 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; const char *from = NULL; - char *buffer; - int ret; + char *buffer = NULL; + bool ret = false; unsigned int flags;

if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;

- if (vshCommandOptString(cmd, "file",&from)<= 0) { - virDomainFree(dom); - return false; - } + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) + goto cleanup;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) { vshReportError(ctl); - virDomainFree(dom); - return false; + goto cleanup; }

if (vshCommandOptBool(cmd, "config")) { @@ -9218,19 +9158,18 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "force")) flags |= VIR_DOMAIN_DEVICE_MODIFY_FORCE;

- ret = virDomainUpdateDeviceFlags(dom, buffer, flags); - VIR_FREE(buffer); - - if (ret< 0) { + if (virDomainUpdateDeviceFlags(dom, buffer, flags)< 0) { vshError(ctl, _("Failed to update device from %s"), from); - virDomainFree(dom); - return false; - } else { - vshPrint(ctl, "%s", _("Device updated successfully\n")); + goto cleanup; }

+ vshPrint(ctl, "%s", _("Device updated successfully\n")); + ret = true; + +cleanup: + VIR_FREE(buffer); virDomainFree(dom); - return true; + return ret; }

/* @@ -9289,15 +9228,13 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) unsigned int flags;

if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;

- if (vshCommandOptString(cmd, "type",&type)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "type",&type)< 0) goto cleanup;

- if (vshCommandOptString(cmd, "mac",&mac)< 0) { - vshError(ctl, "%s", _("missing option")); + if (vshCommandOptStringReq(ctl, cmd, "mac",&mac)< 0) goto cleanup; - }

doc = virDomainGetXMLDesc(dom, 0); if (!doc) @@ -9389,8 +9326,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) }

cleanup: - if (dom) - virDomainFree(dom); + virDomainFree(dom); xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); @@ -9660,9 +9596,9 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) xmlNodePtr disk_node = NULL;

if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;

- if (vshCommandOptString(cmd, "target",&target)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "target",&target)< 0) goto cleanup;

doc = virDomainGetXMLDesc(dom, 0); @@ -9698,8 +9634,7 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) xmlFreeNode(disk_node); VIR_FREE(disk_xml); VIR_FREE(doc); - if (dom) - virDomainFree(dom); + virDomainFree(dom); return functionReturn; }

@@ -9887,10 +9822,10 @@ cmdChangeMedia(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup;

- if (vshCommandOptString(cmd, "path",&path)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "path",&path)< 0) goto cleanup;

- if (vshCommandOptString(cmd, "source",&source)< 0) + if (vshCommandOptStringReq(ctl, cmd, "source",&source)< 0) goto cleanup;

if (insert&& !source) { @@ -9970,10 +9905,8 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd) goto cleanup; }

- if (vshCommandOptString(cmd, "mountpoint",&mountPoint)< 0) { - vshError(ctl, _("Unable to parse mountpoint parameter")); + if (vshCommandOptStringReq(ctl, cmd, "mountpoint",&mountPoint)< 0) goto cleanup; - }

if (virDomainFSTrim(dom, mountPoint, minimum, flags)< 0) { vshError(ctl, _("Unable to invoke fstrim"));

ACK with the one "<= 0" fixed.

On 2013年01月22日 02:07, Peter Krempa wrote:

--- tools/virsh-host.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tools/virsh-host.c b/tools/virsh-host.c index d05e435..5c33c2d 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -103,10 +103,9 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd) }

VIR_FREE(ctl->name); - if (vshCommandOptString(cmd, "name",&name)< 0) { - vshError(ctl, "%s", _("Please specify valid connection URI")); + if (vshCommandOptStringReq(ctl, cmd, "name",&name)< 0) return false; - } + ctl->name = vshStrdup(ctl, name);

ctl->useGetInfo = false; @@ -550,10 +549,8 @@ cmdNodeSuspend(vshControl *ctl, const vshCmd *cmd) unsigned int suspendTarget; long long duration;

- if (vshCommandOptString(cmd, "target",&target)< 0) { - vshError(ctl, _("Invalid target argument")); + if (vshCommandOptStringReq(ctl, cmd, "target",&target)< 0) return false; - }

if (vshCommandOptLongLong(cmd, "duration",&duration)< 0) { vshError(ctl, _("Invalid duration argument"));

ACK

On 2013年01月22日 02:07, Peter Krempa wrote:

--- tools/virsh-network.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 9ba3ca8..0843a6b 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -50,7 +50,7 @@ vshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd, if (!vshCmdHasOption(ctl, cmd, optname)) return NULL;

- if (vshCommandOptString(cmd, optname,&n)<= 0) + if (vshCommandOptStringReq(ctl, cmd, optname,&n)< 0) return NULL;

vshDebug(ctl, VSH_ERR_INFO, "%s: found option<%s>: %s\n", @@ -158,7 +158,7 @@ cmdNetworkCreate(vshControl *ctl, const vshCmd *cmd) bool ret = true; char *buffer;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) @@ -204,7 +204,7 @@ cmdNetworkDefine(vshControl *ctl, const vshCmd *cmd) bool ret = true; char *buffer;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) @@ -889,10 +889,8 @@ cmdNetworkUpdate(vshControl *ctl, const vshCmd *cmd) if (!(network = vshCommandOptNetwork(ctl, cmd, NULL))) goto cleanup;

- if (vshCommandOptString(cmd, "command",&commandStr)< 0) { - vshError(ctl, "%s", _("missing or malformed command argument")); + if (vshCommandOptStringReq(ctl, cmd, "command",&commandStr)< 0) goto cleanup; - }

if (STREQ(commandStr, "add")) { /* "add" is a synonym for "add-last" */ @@ -905,10 +903,9 @@ cmdNetworkUpdate(vshControl *ctl, const vshCmd *cmd) } }

- if (vshCommandOptString(cmd, "section",§ionStr)< 0) { - vshError(ctl, "%s", _("missing or malformed section argument")); + if (vshCommandOptStringReq(ctl, cmd, "section",§ionStr)< 0) goto cleanup; - } + section = virNetworkSectionTypeFromString(sectionStr); if (section<= 0 || section>= VIR_NETWORK_SECTION_LAST) { vshError(ctl, _("unrecognized section name '%s'"), sectionStr); @@ -927,10 +924,8 @@ cmdNetworkUpdate(vshControl *ctl, const vshCmd *cmd) * the desired xml. */

- if (vshCommandOptString(cmd, "xml",&xml)< 0) { - vshError(ctl, "%s", _("malformed or missing xml argument")); + if (vshCommandOptStringReq(ctl, cmd, "xml",&xml)< 0) goto cleanup; - }

if (*xml != '<') { /* contents of xmldata is actually the name of a file that

ACK

On 2013年01月22日 02:07, Peter Krempa wrote:

--- tools/virsh-interface.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 57cfc4d..5337f5e 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -51,7 +51,7 @@ vshCommandOptInterfaceBy(vshControl *ctl, const vshCmd *cmd, if (!vshCmdHasOption(ctl, cmd, optname)) return NULL;

- if (vshCommandOptString(cmd, optname,&n)<= 0) + if (vshCommandOptStringReq(ctl, cmd, optname,&n)< 0) return NULL;

vshDebug(ctl, VSH_ERR_INFO, "%s: found option<%s>: %s\n", @@ -503,7 +503,7 @@ cmdInterfaceDefine(vshControl *ctl, const vshCmd *cmd) bool ret = true; char *buffer;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) @@ -778,10 +778,8 @@ cmdInterfaceBridge(vshControl *ctl, const vshCmd *cmd) }

/* Name for new bridge device */ - if (vshCommandOptString(cmd, "bridge",&br_name)<= 0) { - vshError(ctl, "%s", _("Missing bridge device name in command")); + if (vshCommandOptStringReq(ctl, cmd, "bridge",&br_name)< 0) goto cleanup; - }

/* make sure "new" device doesn't already exist */ if ((br_handle = virInterfaceLookupByName(ctl->conn, br_name))) {

ACK

On 2013年01月22日 02:07, Peter Krempa wrote:

--- tools/virsh-nodedev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 11135ab..8b4c75b 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -67,7 +67,7 @@ cmdNodeDeviceCreate(vshControl *ctl, const vshCmd *cmd) bool ret = true; char *buffer;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) @@ -115,7 +115,7 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) bool ret = true; const char *name = NULL;

- if (vshCommandOptString(cmd, "name",&name)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "name",&name)< 0) return false;

dev = virNodeDeviceLookupByName(ctl->conn, name); @@ -488,7 +488,7 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) virNodeDevicePtr device; char *xml;

- if (vshCommandOptString(cmd, "device",&name)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "device",&name)< 0) return false; if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { vshError(ctl, "%s '%s'", _("Could not find matching device"), name); @@ -533,7 +533,7 @@ cmdNodeDeviceDetach(vshControl *ctl, const vshCmd *cmd) virNodeDevicePtr device; bool ret = true;

- if (vshCommandOptString(cmd, "device",&name)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "device",&name)< 0) return false; if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { vshError(ctl, "%s '%s'", _("Could not find matching device"), name); @@ -578,7 +578,7 @@ cmdNodeDeviceReAttach(vshControl *ctl, const vshCmd *cmd) virNodeDevicePtr device; bool ret = true;

- if (vshCommandOptString(cmd, "device",&name)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "device",&name)< 0) return false; if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { vshError(ctl, "%s '%s'", _("Could not find matching device"), name); @@ -621,7 +621,7 @@ cmdNodeDeviceReset(vshControl *ctl, const vshCmd *cmd) virNodeDevicePtr device; bool ret = true;

- if (vshCommandOptString(cmd, "device",&name)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "device",&name)< 0) return false; if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { vshError(ctl, "%s '%s'", _("Could not find matching device"), name);

ACK

On 2013年01月22日 02:07, Peter Krempa wrote:

This patch adds some empty lines to separate blocks of code, cleans up unnecessary error message constructs in cmdNodeDeviceDetach, cmdNodeDeviceReAttach, cmdNodeDeviceReset and refactors error paths in cmdNodeDeviceDumpXML. --- tools/virsh-nodedev.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 8b4c75b..f85bded 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -485,26 +485,28 @@ static bool cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) { const char *name = NULL; - virNodeDevicePtr device; - char *xml; + virNodeDevicePtr device = NULL; + char *xml = NULL; + bool ret = false;

if (vshCommandOptStringReq(ctl, cmd, "device",&name)< 0) return false; + if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), name); + vshError(ctl, _("Could not find matching device '%s'"), name); return false; }

- xml = virNodeDeviceGetXMLDesc(device, 0); - if (!xml) { - virNodeDeviceFree(device); - return false; - } + if (!(xml = virNodeDeviceGetXMLDesc(device, 0))) + goto cleanup;

vshPrint(ctl, "%s\n", xml); + ret = true; + +cleanup: VIR_FREE(xml); virNodeDeviceFree(device); - return true; + return ret; }

/* @@ -535,8 +537,9 @@ cmdNodeDeviceDetach(vshControl *ctl, const vshCmd *cmd)

if (vshCommandOptStringReq(ctl, cmd, "device",&name)< 0) return false; + if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), name); + vshError(ctl, _("Could not find matching device '%s'"), name); return false; }

@@ -548,6 +551,7 @@ cmdNodeDeviceDetach(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("Failed to detach device %s"), name); ret = false; } + virNodeDeviceFree(device); return ret; } @@ -580,8 +584,9 @@ cmdNodeDeviceReAttach(vshControl *ctl, const vshCmd *cmd)

if (vshCommandOptStringReq(ctl, cmd, "device",&name)< 0) return false; + if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), name); + vshError(ctl, _("Could not find matching device '%s'"), name); return false; }

@@ -591,6 +596,7 @@ cmdNodeDeviceReAttach(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("Failed to re-attach device %s"), name); ret = false; } + virNodeDeviceFree(device); return ret; } @@ -623,8 +629,9 @@ cmdNodeDeviceReset(vshControl *ctl, const vshCmd *cmd)

if (vshCommandOptStringReq(ctl, cmd, "device",&name)< 0) return false; + if (!(device = virNodeDeviceLookupByName(ctl->conn, name))) { - vshError(ctl, "%s '%s'", _("Could not find matching device"), name); + vshError(ctl, _("Could not find matching device '%s'"), name); return false; }

@@ -634,6 +641,7 @@ cmdNodeDeviceReset(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("Failed to reset device %s"), name); ret = false; } + virNodeDeviceFree(device); return ret; }

ACK

On 2013年01月22日 02:07, Peter Krempa wrote:

This patch simplifies error paths and switches to use vshCommandOptStringReq for argument retrieval in cmdSnapshotDumpXML --- tools/virsh-snapshot.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index f76c40e..c980b5b 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1775,31 +1775,26 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "security-info")) flags |= VIR_DOMAIN_XML_SECURE;

- dom = vshCommandOptDomain(ctl, cmd, NULL); - if (dom == NULL) - goto cleanup; + if (vshCommandOptStringReq(ctl, cmd, "snapshotname",&name)< 0) + return false;

- if (vshCommandOptString(cmd, "snapshotname",&name)<= 0) - goto cleanup; + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false;

- snapshot = virDomainSnapshotLookupByName(dom, name, 0); - if (snapshot == NULL) + if (!(snapshot = virDomainSnapshotLookupByName(dom, name, 0))) goto cleanup;

- xml = virDomainSnapshotGetXMLDesc(snapshot, flags); - if (!xml) + if (!(xml = virDomainSnapshotGetXMLDesc(snapshot, flags))) goto cleanup;

vshPrint(ctl, "%s", xml); - ret = true;

cleanup: VIR_FREE(xml); if (snapshot) virDomainSnapshotFree(snapshot); - if (dom) - virDomainFree(dom); + virDomainFree(dom);

return ret; }

ACK

On 2013年01月22日 02:07, Peter Krempa wrote:

also avoids potential NULL pointer dereference:

$ virsh snapshot-current asdf "" error: invalid snapshotname argument '(null)'

by removing the error message in favor of vshCommandOptStringReq --- tools/virsh-snapshot.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index c980b5b..ba57059 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -207,7 +207,7 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup;

- if (vshCommandOptString(cmd, "xmlfile",&from)<= 0) { + if (vshCommandOptStringReq(ctl, cmd, "xmlfile",&from)< 0) { buffer = vshStrdup(ctl, "<domainsnapshot/>"); } else { if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) { @@ -431,11 +431,9 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup;

- if (vshCommandOptString(cmd, "name",&name)< 0 || - vshCommandOptString(cmd, "description",&desc)< 0) { - vshError(ctl, _("argument must not be empty")); + if (vshCommandOptStringReq(ctl, cmd, "name",&name)< 0 || + vshCommandOptStringReq(ctl, cmd, "description",&desc)< 0) goto cleanup; - }

virBufferAddLit(&buf, "<domainsnapshot>\n"); if (name) @@ -443,10 +441,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) if (desc) virBufferEscapeString(&buf, "<description>%s</description>\n", desc);

- if (vshCommandOptString(cmd, "memspec",&memspec)< 0) { - vshError(ctl, _("memspec argument must not be empty")); + if (vshCommandOptStringReq(ctl, cmd, "memspec",&memspec)< 0) goto cleanup; - }

if (memspec&& vshParseSnapshotMemspec(ctl,&buf, memspec)< 0) goto cleanup; @@ -496,10 +492,8 @@ vshLookupSnapshot(vshControl *ctl, const vshCmd *cmd, bool current = vshCommandOptBool(cmd, "current"); const char *snapname = NULL;

- if (vshCommandOptString(cmd, arg,&snapname)< 0) { - vshError(ctl, _("invalid argument for --%s"), arg); + if (vshCommandOptStringReq(ctl, cmd, arg,&snapname)< 0) return -1; - }

if (exclusive&& current&& snapname) { vshError(ctl, _("--%s and --current are mutually exclusive"), arg); @@ -703,10 +697,9 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup;

- if (vshCommandOptString(cmd, "snapshotname",&snapshotname)< 0) { - vshError(ctl, _("invalid snapshotname argument '%s'"), snapshotname); + if (vshCommandOptStringReq(ctl, cmd, "snapshotname",&snapshotname)< 0) goto cleanup; - } + if (snapshotname) { virDomainSnapshotPtr snapshot2 = NULL; flags = (VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |

ACK

On 2013年01月22日 02:07, Peter Krempa wrote:

--- tools/virsh-nwfilter.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c index 61ec34c..9407d38 100644 --- a/tools/virsh-nwfilter.c +++ b/tools/virsh-nwfilter.c @@ -49,7 +49,7 @@ vshCommandOptNWFilterBy(vshControl *ctl, const vshCmd *cmd, if (!vshCmdHasOption(ctl, cmd, optname)) return NULL;

- if (vshCommandOptString(cmd, optname,&n)<= 0) + if (vshCommandOptStringReq(ctl, cmd, optname,&n)< 0) return NULL;

vshDebug(ctl, VSH_ERR_INFO, "%s: found option<%s>: %s\n", @@ -103,7 +103,7 @@ cmdNWFilterDefine(vshControl *ctl, const vshCmd *cmd) bool ret = true; char *buffer;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0)

ACK

On 2013年01月22日 02:07, Peter Krempa wrote:

This patch switches string option retrieval to vshCommandOptStringReq and refactors some error paths. --- tools/virsh-secret.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 05bab85..a735498 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -48,7 +48,7 @@ vshCommandOptSecret(vshControl *ctl, const vshCmd *cmd, const char **name) if (!vshCmdHasOption(ctl, cmd, optname)) return NULL;

- if (vshCommandOptString(cmd, optname,&n)<= 0) + if (vshCommandOptStringReq(ctl, cmd, optname,&n)< 0) return NULL;

vshDebug(ctl, VSH_ERR_DEBUG, @@ -90,28 +90,31 @@ cmdSecretDefine(vshControl *ctl, const vshCmd *cmd) char *buffer; virSecretPtr res; char uuid[VIR_UUID_STRING_BUFLEN]; + bool ret = false;

- if (vshCommandOptString(cmd, "file",&from)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "file",&from)< 0) return false;

if (virFileReadAll(from, VSH_MAX_XML_FILE,&buffer)< 0) return false;

- res = virSecretDefineXML(ctl->conn, buffer, 0); - VIR_FREE(buffer); - - if (res == NULL) { + if (!(res = virSecretDefineXML(ctl->conn, buffer, 0))) { vshError(ctl, _("Failed to set attributes from %s"), from); - return false; + goto cleanup; } + if (virSecretGetUUIDString(res,&(uuid[0]))< 0) { vshError(ctl, "%s", _("Failed to get UUID of created secret")); - virSecretFree(res); - return false; + goto cleanup; } + vshPrint(ctl, _("Secret %s created\n"), uuid); + ret = true; + +cleanup: + VIR_FREE(buffer); virSecretFree(res); - return true; + return ret; }

/* @@ -188,11 +191,10 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) int res; bool ret = false;

- secret = vshCommandOptSecret(ctl, cmd, NULL); - if (secret == NULL) + if (!(secret = vshCommandOptSecret(ctl, cmd, NULL))) return false;

- if (vshCommandOptString(cmd, "base64",&base64)<= 0) + if (vshCommandOptStringReq(ctl, cmd, "base64",&base64)< 0) goto cleanup;

if (!base64_decode_alloc(base64, strlen(base64),&value,&value_size)) { @@ -201,7 +203,7 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) } if (value == NULL) { vshError(ctl, "%s", _("Failed to allocate memory")); - return false; + goto cleanup;

Good to say this fixes a memory leak in commit msg, ACK.