Re: [libvirt] [PATCH 6/8] qemuDomainCreateDeviceRecursive: Support file mount points

On 06/22/2017 12:18 PM, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1462060 > > When building a qemu namespace we might be dealing with bare > regular files. Files that live under /dev. For instance > /dev/my_awesome_disk: > > <disk type='file' device='disk'> > <driver name='qemu' type='qcow2'/> > <source file='/dev/my_awesome_disk'/> > <target dev='vdc' bus='virtio'/> > </disk> > > # qemu-img create -f qcow2 /dev/my_awesome_disk 10M > > So far we were mknod()-ing them which is > obviously wrong. We need to touch the file and bind mount it to > the original: > > 1) touch /var/run/libvirt/qemu/fedora.dev/my_awesome_disk > 2) mount --bind /dev/my_awesome_disk /var/run/libvirt/qemu/fedora.dev/my_awesome_disk > > Later, when the new /dev is built and replaces original /dev the > file is going to live at expected location. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/qemu/qemu_domain.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 977b5c089..6d7c218a2 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -7708,6 +7708,7 @@ qemuDomainCreateDeviceRecursive(const char *device, > int ret = -1; > bool isLink = false; > bool isDev = false; > + bool isReg = false; > bool create = false; > #ifdef WITH_SELINUX > char *tcon = NULL; > @@ -7731,6 +7732,7 @@ qemuDomainCreateDeviceRecursive(const char *device, > > isLink = S_ISLNK(sb.st_mode); > isDev = S_ISCHR(sb.st_mode) || S_ISBLK(sb.st_mode); > + isReg = S_ISREG(sb.st_mode); > > /* Here, @device might be whatever path in the system. We > * should create the path in the namespace iff it's "/dev" > @@ -7842,16 +7844,12 @@ qemuDomainCreateDeviceRecursive(const char *device, > } > goto cleanup; > } > - > - /* Set the file permissions again: mknod() is affected by the > - * current umask, and as such might not have set them correctly */ > + } else if (isReg) { > if (create && > - chmod(devicePath, sb.st_mode) < 0) { > - virReportSystemError(errno, > - _("Failed to set permissions for device %s"), > - devicePath); > + virFileTouch(devicePath, sb.st_mode) < 0) > goto cleanup; > - } > + /* Just create the file here so that code below sets > + * proper owner and mode. Bind mount only after that. */ > } else { > virReportError(VIR_ERR_OPERATION_UNSUPPORTED, > _("unsupported device type %s %o"), > @@ -7871,6 +7869,15 @@ qemuDomainCreateDeviceRecursive(const char *device, > goto cleanup; > } > + /* Symlinks don't have mode */ > + if (!isLink && So the "one" concern I have would be to use (isDev || isReg) instead of (!isLink) - if only to CYA that something new bool isn't invented that would also not need the chmod. IDC, I'm fine with it this way - your call - just figured I'd point it out.

Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt;