Superflouous memcpy() in vmctl/main.c

[Preben Guldberg]

Looking through the code for vmctl, I came across a repeated memcpy() in
vmctl/main.c.

In the checks below, ret is  either set by a memcpy() or defaulted to 0.

If set by memcpy(), and ret != 0, the memcpy() is repeated verbatim,
which seems unnecessary.

diff 09b708f572d76de8db7f7948ea7359b19bbd1c5a /usr/src
blob - 249eaa3ded1ee9c804a81874613c292a74ea4b21
file + usr.sbin/vmctl/main.c
--- usr.sbin/vmctl/main.c
+++ usr.sbin/vmctl/main.c
@@ -300,13 +300,12 @@ vmmaction(struct parse_result *res)
if (imsg.hdr.type == IMSG_CTL_FAIL) {
if (IMSG_DATA_SIZE(&imsg) == sizeof(ret))
memcpy(&ret, imsg.data, sizeof(ret));
else
ret = 0;
if (ret != 0) {
-   memcpy(&ret, imsg.data, sizeof(ret));
errno = ret;
err(1, "command failed");
} else
errx(1, "command failed");
}
 

Re: vmctl: off-by-one error handling mixing -a with a VM id

[Preben Guldberg]

Theo Buehler wrote:
> 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]);
>   }

FWIW, I agree this is clearer.

Thanks for the feedback.

Preben

Re: vmctl: off-by-one error handling mixing -a with a VM id

[Preben Guldberg]

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]

diff 09b708f572d76de8db7f7948ea7359b19bbd1c5a /usr/src
blob - 249eaa3ded1ee9c804a81874613c292a74ea4b21
file + usr.sbin/vmctl/main.c
--- usr.sbin/vmctl/main.c
+++ usr.sbin/vmctl/main.c
@@ -961,7 +961,7 @@ ctl_stop(struct parse_result *res, int argc, char *arg
/* 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]);
+   ctl_usage(res->ctl);
 
return (vmmaction(res));
 }

vmctl: off-by-one error handling mixing -a with a VM id

[Preben Guldberg]

Thanks to Dave, Theo and Klemens for accepting my previous patch

In other tests, I noticed the following error:

% vmctl stop -a testvm
vmctl: invalid id: (null)

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.

Index: usr.sbin/vmctl/main.c
===
RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
retrieving revision 1.62
diff -u -p -u -p -r1.62 main.c
--- usr.sbin/vmctl/main.c   3 Jan 2020 05:32:00 -   1.62
+++ usr.sbin/vmctl/main.c   12 Mar 2021 11:23:26 -
@@ -961,7 +961,7 @@ ctl_stop(struct parse_result *res, int a
/* 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]);
+   errx(1, "invalid use of id with -a: %s", argv[0]);
 
return (vmmaction(res));
 }

vmctl status does not reflect the stopping state of a VM

[Preben Guldberg]

In "vmctl status", VMs that are being stopped but are still running
will simply show up as "running".

The diff below gives preference to showing the stopping state akin to
how a paused VM is handled.

Index: usr.sbin/vmctl/vmctl.c
===
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.c,v
retrieving revision 1.76
diff -u -p -u -p -r1.76 vmctl.c
--- usr.sbin/vmctl/vmctl.c  27 Jan 2021 07:21:12 -  1.76
+++ usr.sbin/vmctl/vmctl.c  7 Mar 2021 15:39:03 -
@@ -708,7 +708,7 @@ add_info(struct imsg *imsg, int *ret)
  *
  * Returns a string representing the current VM state, note that the order
  * matters. A paused VM does have the VM_STATE_RUNNING bit set, but
- * VM_STATE_PAUSED is more significant to report.
+ * VM_STATE_PAUSED is more significant to report. Same goes for stopping VMs.
  *
  * Parameters
  *  vm_state: mask indicating the vm state
@@ -720,10 +720,10 @@ vm_state(unsigned int mask)
return "paused";
else if (mask & VM_STATE_WAITING)
return "waiting";
-   else if (mask & VM_STATE_RUNNING)
-   return "running";
else if (mask & VM_STATE_SHUTDOWN)
return "stopping";
+   else if (mask & VM_STATE_RUNNING)
+   return "running";
/* Presence of absence of other flags */
else if (!mask || (mask & VM_STATE_DISABLED))
return "stopped";