James Cook writes:
> Currently, "vmctl create -i source.qcow2 dest.qcow" fails when
> source.qcow2 has a base image, because virtio_qcow2_get_base calls
> realpath which doesn't interact well with unveil.
>
> The below patch fixes it by delaying the first call to unveil. Caveat:
> I have never worked with unveil or any of this code before, but the
> change seems simple enough.
I believe the correct fix here is to unveil the base image in the
virtio_qcow2_get_base function in vioqcow2.c and not move around all the
unveil calls like you're doing. There's no reason to postpone them.
I think virtio_qcow2_get_base is only used by vmctl (haven't confirmed)
but if so it should be safe to add the unveil call there. I did a quick
1-line change and checked that it at least resolves the error you're
reporting.
>
> I also documented a quirk of the "vmctl create" command: when the -b
> option is specified, the "disk" argument had better be in the current
> directory. (Otherwise for example vmd won't be able to open the image.)
>
You mention both vmctl(8) and vmd(8). Where is the problem? When you
start a guest with vmd(8) or when you're running "vmctl create"?
> The patch includes a regression test that demonstrates the existing
> problem.
>
>
>
> diff --git a/regress/usr.sbin/Makefile b/regress/usr.sbin/Makefile
> index 60e2178d3c7..146f9c9f322 100644
> --- a/regress/usr.sbin/Makefile
> +++ b/regress/usr.sbin/Makefile
> @@ -15,6 +15,7 @@ SUBDIR += rpki-client
> SUBDIR += snmpd
> SUBDIR += switchd
> SUBDIR += syslogd
> +SUBDIR += vmctl
>
> .if ${MACHINE} == "amd64" || ${MACHINE} == "i386"
> SUBDIR += vmd
> diff --git a/regress/usr.sbin/vmctl/Makefile b/regress/usr.sbin/vmctl/Makefile
> new file mode 100644
> index 00000000000..108ff0a43c4
> --- /dev/null
> +++ b/regress/usr.sbin/vmctl/Makefile
> @@ -0,0 +1,18 @@
> +# $OpenBSD$
> +
> +REGRESS_TARGETS=convert_with_absolute_base_path convert_with_local_base_paths
> +
> +convert_with_absolute_base_path:
> + rm *.qcow2
> + vmctl create -s 1m base.qcow2
> + vmctl create -b ${PWD}/base.qcow2 source.qcow2
> + vmctl create -i source.qcow2 dest.qcow2
> +
> +convert_with_local_base_paths:
> + rm *.qcow2
> + vmctl create -s 1m base0.qcow2
> + vmctl create -b base0.qcow2 base1.qcow2
> + vmctl create -b base1.qcow2 source.qcow2
> + vmctl create -i source.qcow2 dest.qcow2
> +
> +.include <bsd.regress.mk>
> diff --git a/usr.sbin/vmctl/main.c b/usr.sbin/vmctl/main.c
> index 249eaa3ded1..2d713d67274 100644
> --- a/usr.sbin/vmctl/main.c
> +++ b/usr.sbin/vmctl/main.c
> @@ -575,8 +575,7 @@ ctl_create(struct parse_result *res, int argc, char
> *argv[])
> break;
> case 'i':
> input = optarg;
> - if (unveil(input, "r") == -1)
> - err(1, "unveil");
> + /* Don't call unveil before ctl_convert. */
> break;
> case 's':
> if (parse_size(res, optarg) != 0)
> @@ -597,8 +596,6 @@ ctl_create(struct parse_result *res, int argc, char
> *argv[])
>
> if (pledge("stdio rpath wpath cpath unveil", NULL) == -1)
> err(1, "pledge");
> - if (unveil(disk, "rwc") == -1)
> - err(1, "unveil");
>
> if (input) {
> if (base && input)
> @@ -606,6 +603,8 @@ ctl_create(struct parse_result *res, int argc, char
> *argv[])
> return ctl_convert(input, disk, type, res->size);
> }
>
> + if (unveil(disk, "rwc") == -1)
> + err(1, "unveil");
> if (unveil(NULL, NULL))
> err(1, "unveil");
>
> @@ -656,7 +655,10 @@ ctl_convert(const char *srcfile, const char *dstfile,
> int dsttype, size_t dstsiz
> goto done;
> }
>
> - /* We can only lock unveil after opening the disk and all base images */
> + /* We can't call unveil before open_imagefile, since it might call
> + virtio_qcow2_get_base. */
> + if (unveil(dst.disk, "rwc") == -1)
> + err(1, "unveil");
> if (unveil(NULL, NULL))
> err(1, "unveil");
>
> diff --git a/usr.sbin/vmctl/vmctl.8 b/usr.sbin/vmctl/vmctl.8
> index d36170e3e24..42cde19544b 100644
> --- a/usr.sbin/vmctl/vmctl.8
> +++ b/usr.sbin/vmctl/vmctl.8
> @@ -77,7 +77,7 @@ connect to the console of the VM with the specified
> Create a VM disk image file with the specified
> .Ar disk
> path.
> -.Bl -tag -width "-i input"
> +.Bl -tag -width "-i disk"
> .It Fl b Ar base
> For
> .Sq qcow2 ,
> @@ -85,7 +85,9 @@ a
> .Ar base
> image may be specified.
> The base image is not modified and the derived image contains only the
> -changes written by the VM.
> +changes written by the VM. When using this option,
> +.Ar disk
> +should be in the current directory.
> .It Fl i Ar disk
> Copy and convert the input
> .Ar disk
> diff --git a/usr.sbin/vmctl/vmctl.c b/usr.sbin/vmctl/vmctl.c
> index d8edbc062eb..3b217121a14 100644
> --- a/usr.sbin/vmctl/vmctl.c
> +++ b/usr.sbin/vmctl/vmctl.c
> @@ -847,6 +847,9 @@ vm_console(struct vmop_info_result *list, size_t ct)
> *
> * Open an imagefile with the specified type, path and size.
> *
> + * Do not call unveil before this function, in case virtio_qcow2_get_base
> + * needs to resolve base images. (This function does call unveil.)
> + *
> * Parameters:
> * type : format of the image file
> * imgfile_path: path to the image file to create
> @@ -885,14 +888,6 @@ open_imagefile(int type, const char *imgfile_path, int
> flags,
> } else if (ret == 0)
> break;
>
> - /*
> - * This might be called after unveil is already
> - * locked but it is save to ignore the EPERM error
> - * here as the subsequent open would fail as well.
> - */
> - if ((ret = unveil(path, "r")) != 0 &&
> - (ret != EPERM))
> - err(1, "unveil");
> if ((basefd[i + 1] = open(path, O_RDONLY)) == -1) {
> log_warn("%s: failed to open base %s",
> __func__, path);
--
-Dave Voutila
