Dave Voutila writes:
> 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.
Slight mispeak missing the keyword "only". '-' is the only case I found
that ONLY triggers "invalid id". Others (e.g. too long a name) also warn
about specific invalid conditions.
>
> 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
