On Wed, 2015-05-27 at 16:47 +0200, Michal Privoznik wrote:
> > 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?
Done. I've only updated the comments for vshCommandOptInt() because all
other functions explicitly refer to that one in their comments.
> 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"));
Agreed, two error messages for a single failure is not something the
user should run into.
I've gone for a slightly different implementation here, which adds two
small commits at the beginning of the series. Please check it out.
> You've missed one ocurrance of vshCommandOptULongLong in cmdAllocpages().
I'd actually missed it the first time around, when I unified all the error
messages. Great catch!
> 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.
I will look into it next, see what I can do :)
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
$ python -c "print('a'.join(['', 'bologn', '@redh', 't.com']))"
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
