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>
---
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>
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);