vmd/vmctl: improve VM name checks/error handling

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

vmd/vmctl: improve VM name checks/error handling

Klemens Nanni-2
vmd(8) does not support numerical names with `start' and `receive'.
It never worked, the manuals are now clearer about this, but error
handling can still be improved:

        $ vmctl start 60 -t test -d 60.qcow2
        vmctl: start vm command failed: No such file or directory

That's from vm_start_complete() in vmctl.c:254 where vmd(8) unexpectedly
returns EPERM after it failed to create the VM.

        $ vmctl receive 60 </dev/null
        vmctl: invalid vm name
        vmctl: invalid id: 60

Here, first parse_vmid(), then it's caller ctl_receive() complains.

parse_vmid()'s last argument is `needname', indicating that the id to be
parsed must not be numerical.

The following diff makes the code check for a letter in the beginning,
adjusts `start' and `receive' set this argument and print only one error.

        $ ./obj/vmctl start 60 -t test -d 60.qcow2
        vmctl: invalid VM name
        $ ./obj/vmctl receive 60 </dev/null
        vmctl: invalid VM name

"vm" -> "VM" for consistency with all other "invalid VM name" occurences
throughout the sources.

Feedback? OK?

Index: usr.sbin/vmctl/main.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
retrieving revision 1.54
diff -u -p -r1.54 main.c
--- usr.sbin/vmctl/main.c 1 Mar 2019 12:47:36 -0000 1.54
+++ usr.sbin/vmctl/main.c 7 Mar 2019 19:14:15 -0000
@@ -524,7 +524,7 @@ parse_vmid(struct parse_result *res, cha
  id = strtonum(word, 0, UINT32_MAX, &error);
  if (error == NULL) {
  if (needname) {
- warnx("invalid vm name");
+ warnx("invalid VM name");
  return (-1);
  } else {
  res->id = id;
@@ -842,8 +842,8 @@ ctl_start(struct parse_result *res, int
  if (argc < 2)
  ctl_usage(res->ctl);
 
- if (parse_vmid(res, argv[1], 0) == -1)
- errx(1, "invalid id: %s", argv[1]);
+ if (parse_vmid(res, argv[1], 1) == -1)
+ exit(1);
 
  argc--;
  argv++;
@@ -1046,7 +1046,7 @@ ctl_receive(struct parse_result *res, in
  err(1, "pledge");
  if (argc == 2) {
  if (parse_vmid(res, argv[1], 1) == -1)
- errx(1, "invalid id: %s", argv[1]);
+ exit(1);
  } else if (argc != 2)
  ctl_usage(res->ctl);
 
Index: usr.sbin/vmctl/vmctl.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.c,v
retrieving revision 1.65
diff -u -p -r1.65 vmctl.c
--- usr.sbin/vmctl/vmctl.c 6 Dec 2018 09:23:15 -0000 1.65
+++ usr.sbin/vmctl/vmctl.c 7 Mar 2019 19:14:15 -0000
@@ -154,12 +154,7 @@ vm_start(uint32_t start_id, const char *
  }
  }
  if (name != NULL) {
- /*
- * Allow VMs names with alphanumeric characters, dot, hyphen
- * and underscore. But disallow dot, hyphen and underscore at
- * the start.
- */
- if (*name == '-' || *name == '.' || *name == '_')
+ if (!isalpha(*name))
  errx(1, "invalid VM name");
 
  for (s = name; *s != '\0'; ++s) {
Index: usr.sbin/vmd/vmd.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
retrieving revision 1.108
diff -u -p -r1.108 vmd.c
--- usr.sbin/vmd/vmd.c 9 Dec 2018 12:26:38 -0000 1.108
+++ usr.sbin/vmd/vmd.c 7 Mar 2019 19:14:15 -0000
@@ -1257,11 +1257,7 @@ vm_register(struct privsep *ps, struct v
     vcp->vcp_ndisks == 0 && strlen(vcp->vcp_cdrom) == 0) {
  log_warnx("no kernel or disk/cdrom specified");
  goto fail;
- } else if (strlen(vcp->vcp_name) == 0) {
- log_warnx("invalid VM name");
- goto fail;
- } else if (*vcp->vcp_name == '-' || *vcp->vcp_name == '.' ||
-    *vcp->vcp_name == '_') {
+ } else if (strlen(vcp->vcp_name) == 0 || !isalpha(*vcp->vcp_name)) {
  log_warnx("invalid VM name");
  goto fail;
  } else {
===================================================================
Stats: --- 15 lines 531 chars
Stats: +++ 6 lines 185 chars
Stats: -9 lines
Stats: -346 chars

Reply | Threaded
Open this post in threaded view
|

Re: vmd/vmctl: improve VM name checks/error handling

Theo Buehler-3
On Thu, Mar 07, 2019 at 08:52:45PM +0100, Klemens Nanni wrote:
> vmd(8) does not support numerical names with `start' and `receive'.
> It never worked, the manuals are now clearer about this, but error
> handling can still be improved:

I'm not sure I understand. This works fine for me with -current from
yesterday (I have vms configured in my vm.conf).

$ vmctl start 1
vmctl: started vm 1 successfully, tty /dev/ttyp0

Please don't disallow or break it.

Reply | Threaded
Open this post in threaded view
|

Re: vmd/vmctl: improve VM name checks/error handling

Mike Larkin-2
In reply to this post by Klemens Nanni-2
On Thu, Mar 07, 2019 at 08:52:45PM +0100, Klemens Nanni wrote:

> vmd(8) does not support numerical names with `start' and `receive'.
> It never worked, the manuals are now clearer about this, but error
> handling can still be improved:
>
> $ vmctl start 60 -t test -d 60.qcow2
> vmctl: start vm command failed: No such file or directory
>
> That's from vm_start_complete() in vmctl.c:254 where vmd(8) unexpectedly
> returns EPERM after it failed to create the VM.
>
> $ vmctl receive 60 </dev/null
> vmctl: invalid vm name
> vmctl: invalid id: 60
>
> Here, first parse_vmid(), then it's caller ctl_receive() complains.
>
> parse_vmid()'s last argument is `needname', indicating that the id to be
> parsed must not be numerical.
>
> The following diff makes the code check for a letter in the beginning,
> adjusts `start' and `receive' set this argument and print only one error.

This is not right. Each started (or defined if in /etc/vm.conf) VM is assigned
an ID. That ID can be used in place of a name. That behaviour needs to continue
to work.

My advice is to just clean up the man page and error messages, nothing more.

-ml

>
> $ ./obj/vmctl start 60 -t test -d 60.qcow2
> vmctl: invalid VM name
> $ ./obj/vmctl receive 60 </dev/null
> vmctl: invalid VM name
>
> "vm" -> "VM" for consistency with all other "invalid VM name" occurences
> throughout the sources.
>
> Feedback? OK?
>
> Index: usr.sbin/vmctl/main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 main.c
> --- usr.sbin/vmctl/main.c 1 Mar 2019 12:47:36 -0000 1.54
> +++ usr.sbin/vmctl/main.c 7 Mar 2019 19:14:15 -0000
> @@ -524,7 +524,7 @@ parse_vmid(struct parse_result *res, cha
>   id = strtonum(word, 0, UINT32_MAX, &error);
>   if (error == NULL) {
>   if (needname) {
> - warnx("invalid vm name");
> + warnx("invalid VM name");
>   return (-1);
>   } else {
>   res->id = id;
> @@ -842,8 +842,8 @@ ctl_start(struct parse_result *res, int
>   if (argc < 2)
>   ctl_usage(res->ctl);
>  
> - if (parse_vmid(res, argv[1], 0) == -1)
> - errx(1, "invalid id: %s", argv[1]);
> + if (parse_vmid(res, argv[1], 1) == -1)
> + exit(1);
>  
>   argc--;
>   argv++;
> @@ -1046,7 +1046,7 @@ ctl_receive(struct parse_result *res, in
>   err(1, "pledge");
>   if (argc == 2) {
>   if (parse_vmid(res, argv[1], 1) == -1)
> - errx(1, "invalid id: %s", argv[1]);
> + exit(1);
>   } else if (argc != 2)
>   ctl_usage(res->ctl);
>  
> Index: usr.sbin/vmctl/vmctl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmctl/vmctl.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 vmctl.c
> --- usr.sbin/vmctl/vmctl.c 6 Dec 2018 09:23:15 -0000 1.65
> +++ usr.sbin/vmctl/vmctl.c 7 Mar 2019 19:14:15 -0000
> @@ -154,12 +154,7 @@ vm_start(uint32_t start_id, const char *
>   }
>   }
>   if (name != NULL) {
> - /*
> - * Allow VMs names with alphanumeric characters, dot, hyphen
> - * and underscore. But disallow dot, hyphen and underscore at
> - * the start.
> - */
> - if (*name == '-' || *name == '.' || *name == '_')
> + if (!isalpha(*name))
>   errx(1, "invalid VM name");
>  
>   for (s = name; *s != '\0'; ++s) {
> Index: usr.sbin/vmd/vmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 vmd.c
> --- usr.sbin/vmd/vmd.c 9 Dec 2018 12:26:38 -0000 1.108
> +++ usr.sbin/vmd/vmd.c 7 Mar 2019 19:14:15 -0000
> @@ -1257,11 +1257,7 @@ vm_register(struct privsep *ps, struct v
>      vcp->vcp_ndisks == 0 && strlen(vcp->vcp_cdrom) == 0) {
>   log_warnx("no kernel or disk/cdrom specified");
>   goto fail;
> - } else if (strlen(vcp->vcp_name) == 0) {
> - log_warnx("invalid VM name");
> - goto fail;
> - } else if (*vcp->vcp_name == '-' || *vcp->vcp_name == '.' ||
> -    *vcp->vcp_name == '_') {
> + } else if (strlen(vcp->vcp_name) == 0 || !isalpha(*vcp->vcp_name)) {
>   log_warnx("invalid VM name");
>   goto fail;
>   } else {
> ===================================================================
> Stats: --- 15 lines 531 chars
> Stats: +++ 6 lines 185 chars
> Stats: -9 lines
> Stats: -346 chars
>

Reply | Threaded
Open this post in threaded view
|

Re: vmd/vmctl: improve VM name checks/error handling

Klemens Nanni-2
In reply to this post by Theo Buehler-3
On Thu, Mar 07, 2019 at 09:00:55PM +0100, Theo Buehler wrote:

> On Thu, Mar 07, 2019 at 08:52:45PM +0100, Klemens Nanni wrote:
> > vmd(8) does not support numerical names with `start' and `receive'.
> > It never worked, the manuals are now clearer about this, but error
> > handling can still be improved:
>
> I'm not sure I understand. This works fine for me with -current from
> yesterday (I have vms configured in my vm.conf).
>
> $ vmctl start 1
> vmctl: started vm 1 successfully, tty /dev/ttyp0
Hm.  That works because the first VM defined in your vm.conf(5) has a
valid non-numerical name and gets assigned ID 1.

With predefined VMs it works as you showed because ctl_start() parses
the name as ID really, that is valid IDs will be accepted although the
vmctl(8) always said `vmctl start name ...' not `vmctl start id ...'.

The problem I faced is creating new VMs on the fly, possibly without
any vm.conf present.  Here the name passed on the command line is used
to create a new VM instead of referencing an existing one.

This never worked but the manual said it would be possible.  With
`receive' it's even more visible, as it never possibly uses the passed
name as ID.

Here's an example:

        $ vmctl create /10.img -s 10M
        vmctl: raw imagefile created
        # cat >/etc/vm.conf <<EOF
        vm "10" {
                disable
                boot "/bsd.rd"
                disk "/10.img"
        }
        vm "a" {
                disable
                boot "/bsd.rd"
                disk "/10.img"
        }
        EOF
        # rcctl restart vmd
        vmd(ok)
        vmd(ok)
        # vmctl show
           ID   PID VCPUS  MAXMEM  CURMEM     TTY        OWNER NAME
            1     -     1    512M       -       -         root 10
            2     -     1    512M       -       -         root a

Note how it fails to start the VM when referenced by its numerical name
"10", but "a", 1 (or 2) work fine:

        # vmctl start 10
        vmctl: could not open disk image(s)

Corresponding vmd log:
        Mar  7 22:16:44 eru vmd[90156]: invalid configuration, no devices

        # vmctl start 1
        vmctl: started vm 1 successfully, tty /dev/ttypo
        # vmctl stop 1 -f
        stopping vm: forced to terminate vm 1
        # vmctl start a
        vmctl: started vm 2 successfully, tty /dev/ttypo

That is wonky, but I see how `start' must indeed accept IDs in the case
of predefined VMs.

I'll work on docs to clarify this further without touching code, thanks.

Reply | Threaded
Open this post in threaded view
|

Re: vmd/vmctl: improve VM name checks/error handling

Klemens Nanni-2
On Thu, Mar 07, 2019 at 10:21:50PM +0100, Klemens Nanni wrote:

> # vmctl start 1
> vmctl: started vm 1 successfully, tty /dev/ttypo
> # vmctl stop 1 -f
> stopping vm: forced to terminate vm 1
> # vmctl start a
> vmctl: started vm 2 successfully, tty /dev/ttypo
>
> That is wonky, but I see how `start' must indeed accept IDs in the case
> of predefined VMs.
>
> I'll work on docs to clarify this further without touching code, thanks.
How about this?

Index: usr.sbin/vmctl/vmctl.8
===================================================================
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v
retrieving revision 1.61
diff -u -p -r1.61 vmctl.8
--- usr.sbin/vmctl/vmctl.8 7 Mar 2019 18:54:05 -0000 1.61
+++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 20:31:07 -0000
@@ -233,6 +233,11 @@ The instance will inherit settings from
 except for exclusive options such as disk, interface lladdr, or
 interface names.
 .El
+.Pp
+For existing VMs,
+.Ar name
+may be the numerical
+.Ar id .
 .It Cm status Op Ar id
 Lists VMs running on the host, optionally listing just the selected VM
 .Ar id .

Reply | Threaded
Open this post in threaded view
|

Re: vmd/vmctl: improve VM name checks/error handling

Jason McIntyre-2
On Sat, Mar 16, 2019 at 09:31:39PM +0100, Klemens Nanni wrote:

> On Thu, Mar 07, 2019 at 10:21:50PM +0100, Klemens Nanni wrote:
> > # vmctl start 1
> > vmctl: started vm 1 successfully, tty /dev/ttypo
> > # vmctl stop 1 -f
> > stopping vm: forced to terminate vm 1
> > # vmctl start a
> > vmctl: started vm 2 successfully, tty /dev/ttypo
> >
> > That is wonky, but I see how `start' must indeed accept IDs in the case
> > of predefined VMs.
> >
> > I'll work on docs to clarify this further without touching code, thanks.
> How about this?
>
> Index: usr.sbin/vmctl/vmctl.8
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v
> retrieving revision 1.61
> diff -u -p -r1.61 vmctl.8
> --- usr.sbin/vmctl/vmctl.8 7 Mar 2019 18:54:05 -0000 1.61
> +++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 20:31:07 -0000
> @@ -233,6 +233,11 @@ The instance will inherit settings from
>  except for exclusive options such as disk, interface lladdr, or
>  interface names.
>  .El
> +.Pp
> +For existing VMs,
> +.Ar name
> +may be the numerical
> +.Ar id .
>  .It Cm status Op Ar id
>  Lists VMs running on the host, optionally listing just the selected VM
>  .Ar id .
>

hi.

i think more properly we should show

        -t id | name

then all the bits that refer to "id" in that doc make sense. and your
text can just say

        Use an existing VM with the specified
        .Ar id
        or
        .Ar name
        as a template ...

jmc

Reply | Threaded
Open this post in threaded view
|

Re: vmd/vmctl: improve VM name checks/error handling

Klemens Nanni-2
On Sat, Mar 16, 2019 at 10:22:14PM +0000, Jason McIntyre wrote:
> i think more properly we should show
>
> -t id | name
It's about referencing the VM to be started itself, not templates.
`-t id' is not possible.

But you pointed out how my addition would errornously imply that, so
change the `start' synopsis from `name' to `id | name' as well as the
command description to differentiate between both cases.

"Starts" to "Start" while here.

Is that clearer?

Index: usr.sbin/vmctl/vmctl.8
===================================================================
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v
retrieving revision 1.61
diff -u -p -r1.61 vmctl.8
--- usr.sbin/vmctl/vmctl.8 7 Mar 2019 18:54:05 -0000 1.61
+++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 23:06:31 -0000
@@ -144,7 +144,7 @@ under the same path.
 An alias for the
 .Cm status
 command.
-.It Xo Cm start Ar name
+.It Xo Cm start Ar id | name
 .Op Fl cL
 .Bk -words
 .Op Fl B Ar device
@@ -157,7 +157,11 @@ command.
 .Op Fl t Ar name
 .Ek
 .Xc
-Starts a VM defined by the specified name and parameters:
+Start a new VM
+.Ar name
+with the specified parameters.
+An existing VM may be referenced by its
+.Ar id .
 .Bl -tag -width "-I parent"
 .It Fl B Ar device
 Force system to boot from the specified device for this boot.
Index: usr.sbin/vmctl/main.c
===================================================================
RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
retrieving revision 1.54
diff -u -p -r1.54 main.c
--- usr.sbin/vmctl/main.c 1 Mar 2019 12:47:36 -0000 1.54
+++ usr.sbin/vmctl/main.c 16 Mar 2019 22:42:24 -0000
@@ -81,7 +81,7 @@ struct ctl_command ctl_commands[] = {
  { "reset", CMD_RESET, ctl_reset, "[all | switches | vms]" },
  { "send", CMD_SEND, ctl_send, "id", 1},
  { "show", CMD_STATUS, ctl_status, "[id]" },
- { "start", CMD_START, ctl_start, "name"
+ { "start", CMD_START, ctl_start, "id | name"
     " [-cL] [-B device] [-b path] [-d disk] [-i count]\n"
     "\t\t[-m size] [-n switch] [-r path] [-t name]" },
  { "status", CMD_STATUS, ctl_status, "[id]" },

Reply | Threaded
Open this post in threaded view
|

Re: vmd/vmctl: improve VM name checks/error handling

Jason McIntyre-2
On Sun, Mar 17, 2019 at 12:21:32AM +0100, Klemens Nanni wrote:
> On Sat, Mar 16, 2019 at 10:22:14PM +0000, Jason McIntyre wrote:
> > i think more properly we should show
> >
> > -t id | name
> It's about referencing the VM to be started itself, not templates.
> `-t id' is not possible.
>

ah, i thought this was -t we were discussing.

> But you pointed out how my addition would errornously imply that, so
> change the `start' synopsis from `name' to `id | name' as well as the
> command description to differentiate between both cases.
>

i don;t understand why you special case "id" in a separate paragraph.
just document "name" and "id" as normal arguments.

> "Starts" to "Start" while here.
>

within the "start" command it would be consistent,  but within the page
less so. i think this should be fixed wholesale, in a separate diff.

> Is that clearer?
>
> Index: usr.sbin/vmctl/vmctl.8
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v
> retrieving revision 1.61
> diff -u -p -r1.61 vmctl.8
> --- usr.sbin/vmctl/vmctl.8 7 Mar 2019 18:54:05 -0000 1.61
> +++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 23:06:31 -0000
> @@ -144,7 +144,7 @@ under the same path.
>  An alias for the
>  .Cm status
>  command.
> -.It Xo Cm start Ar name
> +.It Xo Cm start Ar id | name
>  .Op Fl cL
>  .Bk -words
>  .Op Fl B Ar device
> @@ -157,7 +157,11 @@ command.
>  .Op Fl t Ar name
>  .Ek
>  .Xc
> -Starts a VM defined by the specified name and parameters:
> +Start a new VM
> +.Ar name
> +with the specified parameters.
> +An existing VM may be referenced by its
> +.Ar id .
>  .Bl -tag -width "-I parent"
>  .It Fl B Ar device
>  Force system to boot from the specified device for this boot.
> Index: usr.sbin/vmctl/main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 main.c
> --- usr.sbin/vmctl/main.c 1 Mar 2019 12:47:36 -0000 1.54
> +++ usr.sbin/vmctl/main.c 16 Mar 2019 22:42:24 -0000
> @@ -81,7 +81,7 @@ struct ctl_command ctl_commands[] = {
>   { "reset", CMD_RESET, ctl_reset, "[all | switches | vms]" },
>   { "send", CMD_SEND, ctl_send, "id", 1},
>   { "show", CMD_STATUS, ctl_status, "[id]" },
> - { "start", CMD_START, ctl_start, "name"
> + { "start", CMD_START, ctl_start, "id | name"
>      " [-cL] [-B device] [-b path] [-d disk] [-i count]\n"
>      "\t\t[-m size] [-n switch] [-r path] [-t name]" },
>   { "status", CMD_STATUS, ctl_status, "[id]" },
>

Reply | Threaded
Open this post in threaded view
|

Re: vmd/vmctl: improve VM name checks/error handling

Klemens Nanni-2
On Sat, Mar 16, 2019 at 11:41:02PM +0000, Jason McIntyre wrote:
> i don;t understand why you special case "id" in a separate paragraph.
Specifying an ID is valid only if you want to start an existing VM.
You cannot create new VMs using a numerical name, that is an ID.  This
limitation is the point I wanted to clarify in the first place.

Without the separate paragraph, this information is lost and `id | name'
could just be `id' again.  But as shown in one of my previous mails,
something like `vmctl start 60 ...' to create a new VM called "60" is
not possible and leads to non-obvious errors, hence my attempt to
differentiate between those cases.

> just document "name" and "id" as normal arguments.
That's what I just did, no?

> within the "start" command it would be consistent,  but within the page
> less so. i think this should be fixed wholesale, in a separate diff.
Sure.

Reply | Threaded
Open this post in threaded view
|

Re: vmd/vmctl: improve VM name checks/error handling

Jason McIntyre-2
On Sun, Mar 17, 2019 at 01:07:23AM +0100, Klemens Nanni wrote:

> On Sat, Mar 16, 2019 at 11:41:02PM +0000, Jason McIntyre wrote:
> > i don;t understand why you special case "id" in a separate paragraph.
> Specifying an ID is valid only if you want to start an existing VM.
> You cannot create new VMs using a numerical name, that is an ID.  This
> limitation is the point I wanted to clarify in the first place.
>
> Without the separate paragraph, this information is lost and `id | name'
> could just be `id' again.  But as shown in one of my previous mails,
> something like `vmctl start 60 ...' to create a new VM called "60" is
> not possible and leads to non-obvious errors, hence my attempt to
> differentiate between those cases.
>
> > just document "name" and "id" as normal arguments.
> That's what I just did, no?
>
> > within the "start" command it would be consistent,  but within the page
> > less so. i think this should be fixed wholesale, in a separate diff.
> Sure.
>

ah, i missed the distinction also. in that case i'm fine with the diff.
i would suggest one tweak, but it's up to you to decide if it reads
better.

> +Start a new VM
> +.Ar name
> +with the specified parameters.
> +An existing VM may be referenced by its
> +.Ar id .

i'd be tempted to phrase it:

        Start a new VM
        .Ar name
        with the specified parameters.
        An existing VM may be started by referencing its
        .Ar id .

jmc