On 07/10/2017 09:33 AM, Michal Privoznik wrote: > 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@xxxxxxxxxx> >>> --- >>> 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. > That's fine - I was 50/50 anyway... John >> >> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> > > Thanks. > > Michal > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
