Re: [libvirt] [PATCH 6/8] qemuDomainCreateDeviceRecursive: Support file mount points
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: >>> >>> >>> >>> >>> >>> >>> >>> # 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 >>> --- >>> 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 > > Thanks. > > Michal > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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: >> >> >> >> >> >> >> >> # 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 >> --- >> 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 Thanks. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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: > > > > > > > > # 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 > --- > 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 John > +chmod(devicePath, sb.st_mode) < 0) { > +virReportSystemError(errno, > + _("Failed to set permissions for device %s"), I'm also OK with printing the permissions "0%o" that failed ;-) > + devicePath); > +goto cleanup; > +} > + > /* Symlinks don't have ACLs. */ > if (!isLink && > virFileCopyACLs(device, devicePath) < 0 && > @@ -7903,6 +7910,11 @@ qemuDomainCreateDeviceRecursive(const char *device, > } > #endif > > +/* Finish mount process started earlier. */ > +if (isReg && > +virFileBindMountDevice(device, devicePath) < 0) > +goto cleanup; > + > ret = 0; > cleanup: > VIR_FREE(target); > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/8] qemuDomainCreateDeviceRecursive: Support file mount points
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: # 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 --- 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 && +chmod(devicePath, sb.st_mode) < 0) { +virReportSystemError(errno, + _("Failed to set permissions for device %s"), + devicePath); +goto cleanup; +} + /* Symlinks don't have ACLs. */ if (!isLink && virFileCopyACLs(device, devicePath) < 0 && @@ -7903,6 +7910,11 @@ qemuDomainCreateDeviceRecursive(const char *device, } #endif +/* Finish mount process started earlier. */ +if (isReg && +virFileBindMountDevice(device, devicePath) < 0) +goto cleanup; + ret = 0; cleanup: VIR_FREE(target); -- 2.13.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list