[libvirt] [PATCH 0/2] Yet another namespace fix, kinda
Fixing easy problem (patch 1) lead me to finding out that there is yet another problem that needs fixing (patch 2). For more information read the code. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434 Martin Kletzander (2): qemu: Don't mangle the storage format for type='dir' qemu: Also treat directories properly when using namespaces src/qemu/qemu_domain.c | 38 +++++++++++++++++++--- src/storage/storage_source.c | 5 +++ .../qemuxml2argv-floppy-drive-noformat.args | 24 ++++++++++++++ .../qemuxml2argv-floppy-drive-noformat.xml | 31 ++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 5 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.xml -- 2.14.1
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/storage/storage_source.c | 5 ++++ .../qemuxml2argv-floppy-drive-noformat.args | 24 +++++++++++++++++ .../qemuxml2argv-floppy-drive-noformat.xml | 31 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 4 files changed, 62 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.xml diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c index b620153f1e5a..bbc5cc77be1a 100644 --- a/src/storage/storage_source.c +++ b/src/storage/storage_source.c @@ -527,11 +527,16 @@ virStorageFileGetMetadata(virStorageSourcePtr src, allow_probe, report_broken); virHashTablePtr cycle = NULL; + virStorageType actualType = virStorageSourceGetActualType(src); int ret = -1; if (!(cycle = virHashCreate(5, NULL))) return -1; + /* No backing chains for type='dir' */ + if (actualType == VIR_STORAGE_TYPE_DIR) + return 0; + if (src->format <= VIR_STORAGE_FILE_NONE) src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.args b/tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.args new file mode 100644 index 000000000000..214067b50345 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot a \ +-usb \ +-drive file=fat:floppy:/var/somefiles,if=none,id=drive-fdc0-0-0,readonly=on \ +-global isa-fdc.driveA=drive-fdc0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.xml b/tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.xml new file mode 100644 index 000000000000..9d9dcfe5e603 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='fd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='dir' device='floppy'> + <source dir='/var/somefiles'/> + <target dev='fda' bus='fdc'/> + <readonly/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='fdc' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 77cfe8d28fd1..b7cf2a22b196 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -887,6 +887,8 @@ mymain(void) QEMU_CAPS_DRIVE_BOOT); DO_TEST("floppy-drive-fat", QEMU_CAPS_DRIVE_BOOT); + DO_TEST("floppy-drive-noformat", + QEMU_CAPS_DRIVE_BOOT); DO_TEST("disk-drive-readonly-disk", QEMU_CAPS_NODEFCONFIG); DO_TEST("disk-drive-readonly-no-device", -- 2.14.1
On 08/18/2017 11:36 AM, Martin Kletzander wrote:
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
How so? Seeing as backing chains weren't mentioned in the bz. If I remove the changes from this patch, the new XML tests still pass (at least for me), so I'm curious at the relationship. Regardless, a bit more beef to the commit message would be nice.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/storage/storage_source.c | 5 ++++ .../qemuxml2argv-floppy-drive-noformat.args | 24 +++++++++++++++++ .../qemuxml2argv-floppy-drive-noformat.xml | 31 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 4 files changed, 62 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-floppy-drive-noformat.xml
diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c index b620153f1e5a..bbc5cc77be1a 100644 --- a/src/storage/storage_source.c +++ b/src/storage/storage_source.c @@ -527,11 +527,16 @@ virStorageFileGetMetadata(virStorageSourcePtr src, allow_probe, report_broken);
virHashTablePtr cycle = NULL; + virStorageType actualType = virStorageSourceGetActualType(src); int ret = -1;
if (!(cycle = virHashCreate(5, NULL))) return -1;
+ /* No backing chains for type='dir' */ + if (actualType == VIR_STORAGE_TYPE_DIR) + return 0; +
This would leak cycle Reviewed-by: John Ferlan <jferlan@redhat.com> With the obvious adjustment and commit message adjustment... John [...]
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 40608554c473..b5fc6697ed29 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7837,6 +7837,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) || S_ISFIFO(sb.st_mode) || S_ISSOCK(sb.st_mode); + isDir = S_ISDIR(sb.st_mode); /* Here, @device might be whatever path in the system. We * should create the path in the namespace iff it's "/dev" @@ -7954,6 +7955,10 @@ qemuDomainCreateDeviceRecursive(const char *device, goto cleanup; /* Just create the file here so that code below sets * proper owner and mode. Bind mount only after that. */ + } else if (isDir) { + if (create && + virFileMakePathWithMode(devicePath, sb.st_mode) < 0) + goto cleanup; } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("unsupported device type %s 0%o"), @@ -8015,7 +8020,7 @@ qemuDomainCreateDeviceRecursive(const char *device, #endif /* Finish mount process started earlier. */ - if (isReg && + if ((isReg || isDir) && virFileBindMountDevice(device, devicePath) < 0) goto cleanup; @@ -8644,6 +8649,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, 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) || S_ISFIFO(data->sb.st_mode) || S_ISSOCK(data->sb.st_mode); + bool isDir = S_ISDIR(data->sb.st_mode); qemuSecurityPostFork(data->driver->securityManager); @@ -8699,6 +8705,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, delDevice = true; /* Just create the file here so that code below sets * proper owner and mode. Move the mount only after that. */ + } else if (isDir) { + /* 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 && errno != EINVAL) { + virReportSystemError(errno, + _("Unable to umount %s"), + data->file); + goto cleanup; + } + if (virFileMakePathWithMode(data->file, data->sb.st_mode) < 0) + goto cleanup; + delDevice = true; + /* Just create the folder 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 0%o"), @@ -8746,14 +8769,18 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED, # endif /* Finish mount process started earlier. */ - if (isReg && + if ((isReg || isDir) && virFileMoveMount(data->target, data->file) < 0) goto cleanup; ret = 0; cleanup: - if (ret < 0 && delDevice) - unlink(data->file); + if (ret < 0 && delDevice) { + if (isDir) + virFileDeleteTree(data->file); + else + unlink(data->file); + } # ifdef WITH_SELINUX freecon(data->tcon); # endif @@ -8798,8 +8825,9 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver, isLink = S_ISLNK(data.sb.st_mode); isReg = S_ISREG(data.sb.st_mode) || S_ISFIFO(data.sb.st_mode) || S_ISSOCK(data.sb.st_mode); + isDir = S_ISDIR(data.sb.st_mode); - if (isReg && STRPREFIX(file, DEVPREFIX)) { + if ((isReg || isDir) && STRPREFIX(file, DEVPREFIX)) { cfg = virQEMUDriverGetConfig(driver); if (!(target = qemuDomainGetPreservedMountPath(cfg, vm, file))) goto cleanup; -- 2.14.1
On Fri, Aug 18, 2017 at 05:36:04PM +0200, Martin Kletzander wrote:
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 40608554c473..b5fc6697ed29 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7837,6 +7837,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) || S_ISFIFO(sb.st_mode) || S_ISSOCK(sb.st_mode); + isDir = S_ISDIR(sb.st_mode);
I forgot to squash this in before sending. It is already part of the patch in my local tree, though. diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c index 3fc1183cbe7a..fd91dc430252 100644 --- i/src/qemu/qemu_domain.c +++ w/src/qemu/qemu_domain.c @@ -7850,6 +7850,7 @@ qemuDomainCreateDeviceRecursive(const char *device, bool isLink = false; bool isDev = false; bool isReg = false; + bool isDir = false; bool create = false; #ifdef WITH_SELINUX char *tcon = NULL; @@ -8840,6 +8841,7 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver, char *target = NULL; bool isLink; bool isReg; + bool isDir; if (!ttl) { virReportSystemError(ELOOP, -- Martin
On 08/18/2017 11:36 AM, Martin Kletzander wrote:
Partially-resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1443434
Similar to 1/2 - it's really not clear what this patch has to do with the referenced bz... Perhaps a few words on the relationship would make things clearer.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_domain.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
John Ferlan -
Martin Kletzander