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

On 06/28/2017 12:11 AM, John Ferlan wrote: > > > 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. Funny, I didn't want to use isDev || isReg for exactly this reason. When new type is introduced nothing needs to be adjusted here. The new type is more likely to support mode - frankly so far symlinks are the only type that I've met that doesn't have mode. Therefore I'd like to keep as is.

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