Re: vmd/vmctl: improve VM name checks/error handling
On Sun, Mar 17, 2019 at 01:07:23AM +0100, Klemens Nanni wrote: > On Sat, Mar 16, 2019 at 11:41:02PM +, 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
Re: vmd/vmctl: improve VM name checks/error handling
On Sat, Mar 16, 2019 at 11:41:02PM +, 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.
Re: vmd/vmctl: improve VM name checks/error handling
On Sun, Mar 17, 2019 at 12:21:32AM +0100, Klemens Nanni wrote:
> On Sat, Mar 16, 2019 at 10:22:14PM +, 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.87 Mar 2019 18:54:05 - 1.61
> +++ usr.sbin/vmctl/vmctl.816 Mar 2019 23:06:31 -
> @@ -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 - 1.54
> +++ usr.sbin/vmctl/main.c 16 Mar 2019 22:42:24 -
> @@ -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]" },
>
Re: vmd/vmctl: improve VM name checks/error handling
On Sat, Mar 16, 2019 at 10:22:14PM +, 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 - 1.61
+++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 23:06:31 -
@@ -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 - 1.54
+++ usr.sbin/vmctl/main.c 16 Mar 2019 22:42:24 -
@@ -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]" },
Re: vmd/vmctl: improve VM name checks/error handling
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.87 Mar 2019 18:54:05 - 1.61 > +++ usr.sbin/vmctl/vmctl.816 Mar 2019 20:31:07 - > @@ -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
Re: vmd/vmctl: improve VM name checks/error handling
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 - 1.61 +++ usr.sbin/vmctl/vmctl.8 16 Mar 2019 20:31:07 - @@ -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 .
Re: vmd/vmctl: improve VM name checks/error handling
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 <
Re: vmd/vmctl: improve VM name checks/error handling
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 60vmctl: 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 60vmctl: 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 - 1.54
> +++ usr.sbin/vmctl/main.c 7 Mar 2019 19:14:15 -
> @@ -524,7 +524,7 @@ parse_vmid(struct parse_result *res, cha
> id = strtonum(word, 0, UINT32_MAX, );
> 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.c6 Dec 2018 09:23:15 - 1.65
> +++ usr.sbin/vmctl/vmctl.c7 Mar 2019 19:14:15 -
> @@ -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.c9 Dec 2018 12:26:38 - 1.108
> +++ usr.sbin/vmd/vmd.c7 Mar 2019 19:14:15 -
> @@ -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
>
Re: vmd/vmctl: improve VM name checks/error handling
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.
vmd/vmctl: improve VM name checks/error handling
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 "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 - 1.54
+++ usr.sbin/vmctl/main.c 7 Mar 2019 19:14:15 -
@@ -524,7 +524,7 @@ parse_vmid(struct parse_result *res, cha
id = strtonum(word, 0, UINT32_MAX, );
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 - 1.65
+++ usr.sbin/vmctl/vmctl.c 7 Mar 2019 19:14:15 -
@@ -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 - 1.108
+++ usr.sbin/vmd/vmd.c 7 Mar 2019 19:14:15 -
@@ -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
