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@ > > 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