On 22.05.2015 10:59, Andrea Bolognani wrote:
--- tests/vcpupin | 4 +- tools/virsh-domain-monitor.c | 9 +-- tools/virsh-domain.c | 134 +++++++------------------------------------ tools/virsh-host.c | 57 +++--------------- tools/virsh-interface.c | 6 +- tools/virsh-network.c | 6 +- tools/virsh-volume.c | 24 ++------ tools/virsh.c | 111 +++++++++++++++++++++-------------- 8 files changed, 104 insertions(+), 247 deletions(-)
Nice cleanup.
diff --git a/tools/virsh.c b/tools/virsh.c index 9f44793..db9354c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1517,23 +1517,27 @@ vshCommandOpt(const vshCmd *cmd, * <0 in all other cases (@value untouched) */
Does it makes sense to note in comments that this function (and others that you're fixing) is reporting an error?
int -vshCommandOptInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd, +vshCommandOptInt(vshControl *ctl, const vshCmd *cmd, const char *name, int *value) { vshCmdOpt *arg; int ret;
- ret = vshCommandOpt(cmd, name, &arg, true); - if (ret <= 0) + if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0) return ret;
- if (virStrToLong_i(arg->data, NULL, 10, value) < 0) - return -1; - return 1; + if ((ret = virStrToLong_i(arg->data, NULL, 10, value)) < 0) + vshError(ctl, + _("Numeric value '%s' for <%s> option is malformed or out of range"), + arg->data, name); + else + ret = 1; + + return ret; }
While reworking this, you've missed vshCommandOptTimeoutToMs() which in case of something like following '--timeout blah' will report error twice: from both OptInt() and OptTimeoutToMs(): error: Numeric value 'blah' for <timeout> option is malformed or out of range error: invalid timeout I think this should be squashed in: @@ -1906,11 +1907,13 @@ vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd, { int rv = vshCommandOptInt(ctl, cmd, "timeout", timeout); - if (rv < 0 || (rv > 0 && *timeout < 1)) { - vshError(ctl, "%s", _("invalid timeout")); + if (rv < 0) return -1; - } if (rv > 0) { + if (*timeout < 1) { + vshError(ctl, "%s", _("invalid timeout")); + return -1; + } /* Ensure that we can multiply by 1000 without overflowing. */ if (*timeout > INT_MAX / 1000) { vshError(ctl, "%s", _("timeout is too big"));
static int -vshCommandOptULongLongInternal(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd, +vshCommandOptULongLongInternal(vshControl *ctl, const vshCmd *cmd, const char *name, unsigned long long *value, bool wrap) { @@ -1754,15 +1767,18 @@ vshCommandOptULongLongInternal(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *c if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0) return ret;
- if (wrap) { - if (virStrToLong_ull(arg->data, NULL, 10, value) < 0) - return -1; - } else { - if (virStrToLong_ullp(arg->data, NULL, 10, value) < 0) - return -1; - } + if (wrap) + ret = virStrToLong_ull(arg->data, NULL, 10, value); + else + ret = virStrToLong_ullp(arg->data, NULL, 10, value); + if (ret < 0) + vshError(ctl, + _("Numeric value '%s' for <%s> option is malformed or out of range"), + arg->data, name); + else + ret = 1;
- return 1; + return ret; }
You've missed one ocurrance of vshCommandOptULongLong in cmdAllocpages().
/** @@ -1812,7 +1828,7 @@ vshCommandOptULongLongWrap(vshControl *ctl, const vshCmd *cmd, * See vshCommandOptInt() */ int -vshCommandOptScaledInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd, +vshCommandOptScaledInt(vshControl *ctl, const vshCmd *cmd, const char *name, unsigned long long *value, int scale, unsigned long long max) { @@ -1825,9 +1841,16 @@ vshCommandOptScaledInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
if (virStrToLong_ull(arg->data, &end, 10, value) < 0 || virScaleInteger(value, end, scale, max) < 0) - return -1; + { + vshError(ctl, + _("Numeric value '%s' for <%s> option is malformed or out of range"), + arg->data, name); + ret = -1; + } else { + ret = 1; + }
- return 1; + return ret; }
Interestingly vshCommandOptString() is missing. So far, the only way for the function to fail is if one uses --option "" and VSH_OFLAG_EMPTY_OK is not specified. So if we come up with good error message for that case, we are okay to drop all the other error messages. Otherwise looking good. Michal