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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list