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