On Sat, Mar 13, 2021 at 08:34:24AM -0500, Dave Voutila wrote: > > 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.
virtio_qcow2_get_base is also called by virtio_get_base in usr.sbin/vmd/virtio.c. Maybe add an "unveil" flag as an additional input to virtio_qcow2_get_base? Is a single call to unveil(path, "r") guaranteed to unveil everything needed to call realpath(path, ...)? I tried to contrive situations involving symlinks inside directories that aren't on the real path, or ".."s, but realpath succeeded in every case I could come up with. > > 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"? Both vmd and vmctl create -i break. As I understand it, if you run vmctl create -b base.qcow2 dir/img.qcow2 then the base image path gets recorded as "base.qcow2", and then virtio_qcow2_get_base expects it to be at dir/base.qcow2 --- see the comment about relative paths in virtio_qcow2_get_base. It would be better to just fix it, but it's not clear to how to do that nicely. In the above example, you'd need to translate "base.qcow2" to "../base.qcow2". If instead it were "vmctl create -b base.qcow2 ../img.qcow2", should you translate that to "<current directory name>/base.qcow2"? As an alternative, you could say the argument to b is relative to the dirname of dir/img.qcow2. So you should run "vmctl create -b ../base.qcow2 dir/img.qcow2" to use ./base.qcow2. I think this would only require a minor change to vmctl. I don't know if it could break any existing scripts, because such scripts should already be broken. If you're happy to just document the quirk for now, I've attached new manpage update --- explains slightly more, and also narrows the comment to be about relative paths. - James diff --git a/usr.sbin/vmctl/vmctl.8 b/usr.sbin/vmctl/vmctl.8 index d36170e3e24..5528f4f5e64 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,12 @@ 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. If +.Ar base +is a relative path, then +.Ar disk +should be in the current directory, otherwise the path to the base +image will be recorded incorrectly. .It Fl i Ar disk Copy and convert the input .Ar disk
