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.
That's right. The warnx() in parse_vmid() should probably be turned into errx() so as to avoid double whining (which is not new with my diff). As far as I can tell, the '-' special case is dead, as -1 from parse_vmid() is always followed by the same errx(). > > 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