On Fri, Mar 26, 2021 at 07:24:32AM -0400, Dave Voutila wrote:
>
> Theo Buehler writes:
>
> > On Thu, Mar 25, 2021 at 08:07:53PM +0100, Preben Guldberg wrote:
> >> Dave Voutila wrote:
> >> > Preben Guldberg writes:
> >> > > The patch below addresses an off-by-one error reading argv when
> >> > > generating the error message.
> >>
> >> > > I personally find it clearer if the condition of mixing -a with an id
> >> > > is highlighted. I included a suggestion in the patch below.
> >>
> >> > Since -a and providing an id are mutually exclusive, I think it's more
> >> > helpful to print usage information via ctl_usage(res->ctl). From the
> >> > usage details, it's self explanatory what's wrong.
> >>
> >> > usage: vmctl [-v] stop [-fw] [id | -a]
> >>
> >> The updated diff below would do just that:
> >>
> >> % vmctl stop -a testvm
> >> usage: vmctl [-v] stop [-fw] [id | -a]
> >
> > Yes, your diff would do that.
> >
> > However, I think the current logic is both wrong and the wrong way
> > around. I believe the following is much clearer. It doesn't have a dead
> > else branch and it deletes 'ret', so it doesn't use it uninitialized when
> > checking 'res->action == CMD_STOPALL && ret != -1' (e.g. 'vmctl stop -a').
> > Since the diff is slightly messy, this is the result:
> >
> > if (res->action == CMD_STOPALL) {
> > if (argc != 0)
> > ctl_usage(res->ctl);
> > } else {
> > if (argc != 1)
> > ctl_usage(res->ctl);
> > if (parse_vmid(res, argv[0], 0) == -1)
> > errx(1, "invalid id: %s", argv[0]);
> > }
> >
> > return (vmmaction(res));
>
> I like this a lot better. The only thing to note is the only code path I
> can identify that will result in "invalid id" is using '-' as the
> id...parse_vmid prints warnings itself for other use cases. Having the
> errx here though is a nice guard if someone changes parse_vmid in the future.
>
> OK dv@
>
also ok mlarkin@
> >
> > Index: main.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
> > retrieving revision 1.62
> > diff -u -p -r1.62 main.c
> > --- main.c 3 Jan 2020 05:32:00 -0000 1.62
> > +++ main.c 25 Mar 2021 19:23:16 -0000
> > @@ -927,7 +927,7 @@ ctl_start(struct parse_result *res, int
> > int
> > ctl_stop(struct parse_result *res, int argc, char *argv[])
> > {
> > - int ch, ret;
> > + int ch;
> >
> > while ((ch = getopt(argc, argv, "afw")) != -1) {
> > switch (ch) {
> > @@ -948,20 +948,15 @@ ctl_stop(struct parse_result *res, int a
> > argc -= optind;
> > argv += optind;
> >
> > - if (argc == 0) {
> > - if (res->action != CMD_STOPALL)
> > + if (res->action == CMD_STOPALL) {
> > + if (argc != 0)
> > ctl_usage(res->ctl);
> > - } else if (argc > 1)
> > - ctl_usage(res->ctl);
> > - else if (argc == 1)
> > - ret = parse_vmid(res, argv[0], 0);
> > - else
> > - ret = -1;
> > -
> > - /* VM id is only expected without the -a flag */
> > - if ((res->action != CMD_STOPALL && ret == -1) ||
> > - (res->action == CMD_STOPALL && ret != -1))
> > - errx(1, "invalid id: %s", argv[1]);
> > + } else {
> > + if (argc != 1)
> > + ctl_usage(res->ctl);
> > + if (parse_vmid(res, argv[0], 0) == -1)
> > + errx(1, "invalid id: %s", argv[0]);
> > + }
> >
> > return (vmmaction(res));
> > }
>
>
> --
> -Dave Voutila
>
