[libvirt] [PATCH 0/3] virsh: error reporting for empty string parameters
https://bugzilla.redhat.com/show_bug.cgi?id=1281707 Ján Tomko (3): virsh: report errors for empty strings virsh: remove custom error for cpulist from cmdIOThreadPin virsh: rename vshCommandOptString to vshCommandOptStringQuiet tools/virsh-domain-monitor.c | 4 ++-- tools/virsh-domain.c | 34 ++++++++++++++++------------------ tools/virsh-network.c | 4 ++-- tools/virsh-nodedev.c | 4 ++-- tools/virsh-volume.c | 2 +- tools/vsh.c | 12 ++++++------ tools/vsh.h | 4 ++-- 7 files changed, 31 insertions(+), 33 deletions(-) -- 2.4.6
Several callers were using vshCommandOptString without setting an error. Use vshCommandOptStringReq which sets the error. https://bugzilla.redhat.com/show_bug.cgi?id=1281707 --- tools/virsh-domain-monitor.c | 4 ++-- tools/virsh-domain.c | 10 +++++----- tools/virsh-network.c | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 64ec03d..7dcf833 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -2243,9 +2243,9 @@ cmdDomIfAddr(vshControl *ctl, const vshCmd *cmd) if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptString(ctl, cmd, "interface", &ifacestr) < 0) + if (vshCommandOptStringReq(ctl, cmd, "interface", &ifacestr) < 0) goto cleanup; - if (vshCommandOptString(ctl, cmd, "source", &sourcestr) < 0) + if (vshCommandOptStringReq(ctl, cmd, "source", &sourcestr) < 0) goto cleanup; if (sourcestr) { diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 413220b..4d79890 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2332,11 +2332,11 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) return false; - if (vshCommandOptString(ctl, cmd, "dest", &dest) < 0) + if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0) return false; - if (vshCommandOptString(ctl, cmd, "xml", &xml) < 0) + if (vshCommandOptStringReq(ctl, cmd, "xml", &xml) < 0) return false; - if (vshCommandOptString(ctl, cmd, "format", &format) < 0) + if (vshCommandOptStringReq(ctl, cmd, "format", &format) < 0) return false; /* XXX: Parse bandwidth as scaled input, rather than forcing * MiB/s, and either reject negative input or treat it as 0 rather @@ -9241,7 +9241,7 @@ cmdQemuMonitorEvent(vshControl *ctl, const vshCmd *cmd) data.count = 0; if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) return false; - if (vshCommandOptString(ctl, cmd, "event", &event) < 0) + if (vshCommandOptStringReq(ctl, cmd, "event", &event) < 0) return false; if (vshCommandOptBool(cmd, "domain")) @@ -12568,7 +12568,7 @@ cmdEvent(vshControl *ctl, const vshCmd *cmd) return true; } - if (vshCommandOptString(ctl, cmd, "event", &eventName) < 0) + if (vshCommandOptStringReq(ctl, cmd, "event", &eventName) < 0) return false; if (eventName) { for (event = 0; event < VIR_DOMAIN_EVENT_ID_LAST; event++) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index a0f7707..bd89c11 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1262,7 +1262,7 @@ cmdNetworkEvent(vshControl *ctl, const vshCmd *cmd) return true; } - if (vshCommandOptString(ctl, cmd, "event", &eventName) < 0) + if (vshCommandOptStringReq(ctl, cmd, "event", &eventName) < 0) return false; if (!eventName) { vshError(ctl, "%s", _("either --list or event type is required")); @@ -1372,7 +1372,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0; virNetworkPtr network = NULL; - if (vshCommandOptString(ctl, cmd, "mac", &mac) < 0) + if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0) return false; if (!(network = virshCommandOptNetwork(ctl, cmd, &name))) -- 2.4.6
Instead of the custom error: error: iothreadpin: invalid cpulist. use vshCommandOptStringReq and let it report a more specific error: error: Failed to get option 'cpulist': Option argument is empty --- tools/virsh-domain.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4d79890..b7e7606 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7116,10 +7116,8 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptUInt(ctl, cmd, "iothread", &iothread_id) < 0) goto cleanup; - if (vshCommandOptString(ctl, cmd, "cpulist", &cpulist) < 0) { - vshError(ctl, "%s", _("iothreadpin: invalid cpulist.")); + if (vshCommandOptStringReq(ctl, cmd, "cpulist", &cpulist) < 0) goto cleanup; - } if ((maxcpu = virshNodeGetCPUCount(priv->conn)) < 0) goto cleanup; -- 2.4.6
This function does not set an error. Make it obvious in its name to discourage its usage without reporting an error in the caller. --- tools/virsh-domain.c | 20 ++++++++++---------- tools/virsh-nodedev.c | 4 ++-- tools/virsh-volume.c | 2 +- tools/vsh.c | 12 ++++++------ tools/vsh.h | 4 ++-- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b7e7606..7650535 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1622,7 +1622,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) goto save_error; } - rv = vshCommandOptString(ctl, cmd, "device-weights", &device_weight); + rv = vshCommandOptStringQuiet(ctl, cmd, "device-weights", &device_weight); if (rv < 0) { vshError(ctl, "%s", _("Unable to parse string parameter")); goto cleanup; @@ -1633,7 +1633,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) goto save_error; } - rv = vshCommandOptString(ctl, cmd, "device-read-iops-sec", &device_riops); + rv = vshCommandOptStringQuiet(ctl, cmd, "device-read-iops-sec", &device_riops); if (rv < 0) { vshError(ctl, "%s", _("Unable to parse string parameter")); goto cleanup; @@ -1644,7 +1644,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) goto save_error; } - rv = vshCommandOptString(ctl, cmd, "device-write-iops-sec", &device_wiops); + rv = vshCommandOptStringQuiet(ctl, cmd, "device-write-iops-sec", &device_wiops); if (rv < 0) { vshError(ctl, "%s", _("Unable to parse string parameter")); goto cleanup; @@ -1655,7 +1655,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) goto save_error; } - rv = vshCommandOptString(ctl, cmd, "device-read-bytes-sec", &device_rbps); + rv = vshCommandOptStringQuiet(ctl, cmd, "device-read-bytes-sec", &device_rbps); if (rv < 0) { vshError(ctl, "%s", _("Unable to parse string parameter")); goto cleanup; @@ -1666,7 +1666,7 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) goto save_error; } - rv = vshCommandOptString(ctl, cmd, "device-write-bytes-sec", &device_wbps); + rv = vshCommandOptStringQuiet(ctl, cmd, "device-write-bytes-sec", &device_wbps); if (rv < 0) { vshError(ctl, "%s", _("Unable to parse string parameter")); goto cleanup; @@ -3736,7 +3736,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) size_t j; virshControlPtr priv = ctl->privData; - ignore_value(vshCommandOptString(ctl, cmd, "storage", &vol_string)); + ignore_value(vshCommandOptStringQuiet(ctl, cmd, "storage", &vol_string)); if (!(vol_string || remove_all_storage) && wipe_storage) { vshError(ctl, @@ -4115,7 +4115,7 @@ cmdStartGetFDs(vshControl *ctl, *nfdsret = 0; *fdsret = NULL; - if (vshCommandOptString(ctl, cmd, "pass-fds", &fdopt) <= 0) + if (vshCommandOptStringQuiet(ctl, cmd, "pass-fds", &fdopt) <= 0) return 0; if (!(fdlist = virStringSplit(fdopt, ",", -1))) { @@ -5310,7 +5310,7 @@ doDump(void *opaque) goto out; } - if (vshCommandOptString(ctl, cmd, "format", &format) > 0) { + if (vshCommandOptStringQuiet(ctl, cmd, "format", &format) > 0) { if (STREQ(format, "kdump-zlib")) { dumpformat = VIR_DOMAIN_CORE_DUMP_FORMAT_KDUMP_ZLIB; } else if (STREQ(format, "kdump-lzo")) { @@ -8359,7 +8359,7 @@ cmdSendKey(vshControl *ctl, const vshCmd *cmd) if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptString(ctl, cmd, "codeset", &codeset_option) <= 0) + if (vshCommandOptStringQuiet(ctl, cmd, "codeset", &codeset_option) <= 0) codeset_option = "linux"; if (vshCommandOptUInt(ctl, cmd, "holdtime", &holdtime) < 0) @@ -8775,7 +8775,7 @@ virshMemtuneGetSize(vshControl *ctl, const vshCmd *cmd, const char *str; char *end; - ret = vshCommandOptString(ctl, cmd, name, &str); + ret = vshCommandOptStringQuiet(ctl, cmd, name, &str); if (ret <= 0) return ret; if (virStrToLong_ll(str, &end, 10, value) < 0) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index cc359e2..bfe507e 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -398,7 +398,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) virshNodeDeviceListPtr list = NULL; int cap_type = -1; - ignore_value(vshCommandOptString(ctl, cmd, "cap", &cap_str)); + ignore_value(vshCommandOptStringQuiet(ctl, cmd, "cap", &cap_str)); if (cap_str) { if (tree) { @@ -615,7 +615,7 @@ cmdNodeDeviceDetach(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "device", &name) < 0) return false; - ignore_value(vshCommandOptString(ctl, cmd, "driver", &driverName)); + ignore_value(vshCommandOptStringQuiet(ctl, cmd, "driver", &driverName)); if (!(device = virNodeDeviceLookupByName(priv->conn, name))) { vshError(ctl, _("Could not find matching device '%s'"), name); diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 7d76a06..3408bee 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -222,7 +222,7 @@ cmdVolCreateAs(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (vshCommandOptString(ctl, cmd, "allocation", &allocationStr) > 0 && + if (vshCommandOptStringQuiet(ctl, cmd, "allocation", &allocationStr) > 0 && virshVolSize(allocationStr, &allocation) < 0) { vshError(ctl, _("Malformed size %s"), allocationStr); goto cleanup; diff --git a/tools/vsh.c b/tools/vsh.c index e57c324..073347a 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -944,21 +944,21 @@ vshCommandOptULWrap(vshControl *ctl, const vshCmd *cmd, } /** - * vshCommandOptString: + * vshCommandOptStringQuiet: * @ctl virtshell control structure * @cmd command reference * @name option name * @value result * - * Returns option as STRING + * Returns option as STRING. On error -1 is returned but no error is set. * Return value: * >0 if option found and valid (@value updated) * 0 if option not found and not required (@value untouched) * <0 in all other cases (@value untouched) */ int -vshCommandOptString(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd, - const char *name, const char **value) +vshCommandOptStringQuiet(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd, + const char *name, const char **value) { vshCmdOpt *arg; int ret; @@ -2793,7 +2793,7 @@ cmdHelp(vshControl *ctl, const vshCmd *cmd) { const char *name = NULL; - if (vshCommandOptString(ctl, cmd, "command", &name) <= 0) { + if (vshCommandOptStringQuiet(ctl, cmd, "command", &name) <= 0) { const vshCmdGrp *grp; const vshCmdDef *def; @@ -2857,7 +2857,7 @@ cmdCd(vshControl *ctl, const vshCmd *cmd) return false; } - if (vshCommandOptString(ctl, cmd, "dir", &dir) <= 0) + if (vshCommandOptStringQuiet(ctl, cmd, "dir", &dir) <= 0) dir = dir_malloced = virGetUserDirectory(); if (!dir) dir = "/"; diff --git a/tools/vsh.h b/tools/vsh.h index fac62f4..9c0d8a6 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -270,8 +270,8 @@ int vshCommandOptUL(vshControl *ctl, const vshCmd *cmd, int vshCommandOptULWrap(vshControl *ctl, const vshCmd *cmd, const char *name, unsigned long *value) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; -int vshCommandOptString(vshControl *ctl, const vshCmd *cmd, - const char *name, const char **value) +int vshCommandOptStringQuiet(vshControl *ctl, const vshCmd *cmd, + const char *name, const char **value) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; int vshCommandOptStringReq(vshControl *ctl, const vshCmd *cmd, const char *name, const char **value) -- 2.4.6
On 03/12/15 14:05, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1281707
Ján Tomko (3): virsh: report errors for empty strings virsh: remove custom error for cpulist from cmdIOThreadPin virsh: rename vshCommandOptString to vshCommandOptStringQuiet
tools/virsh-domain-monitor.c | 4 ++-- tools/virsh-domain.c | 34 ++++++++++++++++------------------ tools/virsh-network.c | 4 ++-- tools/virsh-nodedev.c | 4 ++-- tools/virsh-volume.c | 2 +- tools/vsh.c | 12 ++++++------ tools/vsh.h | 4 ++-- 7 files changed, 31 insertions(+), 33 deletions(-)
ACK series. Erik
participants (2)
-
Erik Skultety -
Ján Tomko