Re: [libvirt] [PATCH 7/8] qemuDomainAttachDeviceMknodRecursive: Support file mount points

[John Ferlan]

On 06/22/2017 12:18 PM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1462060
> 
> Just like in the previous commit, when attaching a file based
> device which has its source living under /dev (that is not a
> device rather than a regular file), calling mknod() is no help.
> We need to:
> 
> 1) bind mount device to some temporary location
> 2) enter the namespace
> 3) move the mount point to desired place
> 4) umount it in the parent namespace from the temporary location
> 
> At the same time, the check in qemuDomainNamespaceSetupDisk makes
> no longer sense. Therefore remove it.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 59 
> --
>  1 file changed, 48 insertions(+), 11 deletions(-)
> 

Reviewed-by: John Ferlan 

John

I have the same !isLink comment as last patch, but I see it's even more
prevalent now so I guess it's fine the way it is...

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 7/8] qemuDomainAttachDeviceMknodRecursive: Support file mount points

[Michal Privoznik]

https://bugzilla.redhat.com/show_bug.cgi?id=1462060

Just like in the previous commit, when attaching a file based
device which has its source living under /dev (that is not a
device rather than a regular file), calling mknod() is no help.
We need to:

1) bind mount device to some temporary location
2) enter the namespace
3) move the mount point to desired place
4) umount it in the parent namespace from the temporary location

At the same time, the check in qemuDomainNamespaceSetupDisk makes
no longer sense. Therefore remove it.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 59 --
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6d7c218a2..51779c535 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8531,6 +8531,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 bool delDevice = false;
 bool isLink = S_ISLNK(data->sb.st_mode);
 bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
+bool isReg = S_ISREG(data->sb.st_mode);
 
 qemuSecurityPostFork(data->driver->securityManager);
 
@@ -8569,6 +8570,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 } else {
 delDevice = true;
 }
+} else if (isReg) {
+/* We are not cleaning up disks on virDomainDetachDevice
+ * because disk might be still in use by different disk
+ * as its backing chain. This might however clash here.
+ * Therefore do the cleanup here. */
+if (umount(data->file) < 0 &&
+errno != ENOENT) {
+virReportSystemError(errno,
+ _("Unable to umount %s"),
+ data->file);
+goto cleanup;
+}
+if (virFileTouch(data->file, data->sb.st_mode) < 0)
+goto cleanup;
+delDevice = true;
+/* Just create the file here so that code below sets
+ * proper owner and mode. Move the mount only after that. */
 } else {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("unsupported device type %s %o"),
@@ -8583,6 +8601,15 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 goto cleanup;
 }
 
+/* Symlinks don't have mode */
+if (!isLink &&
+chmod(data->file, data->sb.st_mode) < 0) {
+virReportSystemError(errno,
+ _("Failed to set permissions for device %s"),
+ data->file);
+goto cleanup;
+}
+
 /* Symlinks don't have ACLs. */
 if (!isLink &&
 virFileSetACLs(data->file, data->acl) < 0 &&
@@ -8606,6 +8633,11 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 }
 #endif
 
+/* Finish mount process started earlier. */
+if (isReg &&
+virFileMoveMount(data->target, data->file) < 0)
+goto cleanup;
+
 ret = 0;
  cleanup:
 if (ret < 0 && delDevice)
@@ -8626,10 +8658,12 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
driver,
  size_t ndevMountsPath,
  unsigned int ttl)
 {
+virQEMUDriverConfigPtr cfg = NULL;
 struct qemuDomainAttachDeviceMknodData data;
 int ret = -1;
 char *target = NULL;
 bool isLink;
+bool isReg;
 
 if (!ttl) {
 virReportSystemError(ELOOP,
@@ -8651,8 +8685,18 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
driver,
 }
 
 isLink = S_ISLNK(data.sb.st_mode);
+isReg = S_ISREG(data.sb.st_mode);
 
-if (isLink) {
+if (isReg && STRPREFIX(file, DEVPREFIX)) {
+cfg = virQEMUDriverGetConfig(driver);
+if (!(target = qemuDomainGetPreservedMountPath(cfg, vm, file)))
+goto cleanup;
+
+if (virFileBindMountDevice(file, target) < 0)
+goto cleanup;
+
+data.target = target;
+} else if (isLink) {
 if (virFileReadLink(file, ) < 0) {
 virReportSystemError(errno,
  _("unable to resolve symlink %s"),
@@ -8739,7 +8783,10 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
driver,
 freecon(data.tcon);
 #endif
 virFileFreeACLs();
+if (isReg && target)
+umount(target);
 VIR_FREE(target);
+virObjectUnref(cfg);
 return ret;
 }
 
@@ -8817,7 +8864,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
 char **devMountsPath = NULL;
 size_t ndevMountsPath = 0;
 virStorageSourcePtr next;
-struct stat sb;
 int ret = -1;
 
 if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
@@ -8836,15 +8882,6 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
 continue;
 }
 
-if (stat(next->path, ) < 0) {
-virReportSystemError(errno,
-