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
