[libvirt] [PATCH 0/4] Fix handling of errors with disk labelling on NFS
The previous commit 06f81c63ebc19cb0e51f9b397991f6d6ae56d090 masked a flaw with handling of errors for NFS disk labels. If you had multiple devices that required labelling, and an disk on root squashing NFS, then when the failure occurred for the NFS file, all further devices would be skipped. The caller would then ignore the error and try to launch the guest anyway, unless there was a stdin_path on NFS. Since NFS error handling was not dealt with in the correct place, this in turn meant that disk hotplug failed with root squashing NFS. All in all a bit of a mess. This tries to clean up the code handling errors closer to the point of occurrance libvirt_private.syms | 1 + qemu/qemu_driver.c | 3 +-- qemu/qemu_monitor_text.c | 28 +++++++++++++++++++--------- security/security_selinux.c | 11 +++++++---- util/storage_file.c | 32 +++++++++++++++++++++++++------- util/storage_file.h | 9 +++++++++ 6 files changed, 62 insertions(+), 22 deletions(-) Daniel
Commit 06f81c63ebc19cb0e51f9b397991f6d6ae56d090 attempted to make QEMU driver ignore the failure to relabel 'stdin_path' if it was on NFS. The actual result was that it ignores *all* failures to label any aspect of the VM, unless stdin_path is non-NULL and is not on NFS. * src/qemu/qemu_driver.c: Treat all relabel failures as terminal --- src/qemu/qemu_driver.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7cce6a..9945f5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3876,8 +3876,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, driver->securityDriver->domainSetSecurityAllLabel && driver->securityDriver->domainSetSecurityAllLabel(driver->securityDriver, vm, stdin_path) < 0) { - if (stdin_path && virStorageFileIsSharedFS(stdin_path) != 1) - goto cleanup; + goto cleanup; } /* Ensure no historical cgroup for this VM is lying around bogus -- 1.7.2.3
On 11/01/2010 11:48 AM, Daniel P. Berrange wrote:
Commit 06f81c63ebc19cb0e51f9b397991f6d6ae56d090 attempted to make QEMU driver ignore the failure to relabel 'stdin_path' if it was on NFS. The actual result was that it ignores *all* failures to label any aspect of the VM, unless stdin_path is non-NULL and is not on NFS.
* src/qemu/qemu_driver.c: Treat all relabel failures as terminal --- src/qemu/qemu_driver.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7cce6a..9945f5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3876,8 +3876,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, driver->securityDriver->domainSetSecurityAllLabel && driver->securityDriver->domainSetSecurityAllLabel(driver->securityDriver, vm, stdin_path) < 0) { - if (stdin_path && virStorageFileIsSharedFS(stdin_path) != 1) - goto cleanup; + goto cleanup;
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
NFS does not support file labelling, so ignore this error for stdin_path when on NFS. * src/security/security_selinux.c: Ignore failures on labelling stdin_path on NFS * src/util/storage_file.c, src/util/storage_file.h: Refine virStorageFileIsSharedFS() to allow it to check for a specific FS type. --- src/libvirt_private.syms | 1 + src/security/security_selinux.c | 9 ++++++--- src/util/storage_file.c | 32 +++++++++++++++++++++++++------- src/util/storage_file.h | 9 +++++++++ 4 files changed, 41 insertions(+), 10 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cf64bd3..003d1a0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -689,6 +689,7 @@ virStorageFileFormatTypeToString; virStorageFileGetMetadata; virStorageFileGetMetadataFromFD; virStorageFileIsSharedFS; +virStorageFileIsSharedFSType; virStorageFileProbeFormat; virStorageFileProbeFormatFromFD; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index a9dd836..0612ce3 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1023,9 +1023,12 @@ SELinuxSetSecurityAllLabel(virSecurityDriverPtr drv, SELinuxSetFilecon(vm->def->os.initrd, default_content_context) < 0) return -1; - if (stdin_path && - SELinuxSetFilecon(stdin_path, default_content_context) < 0) - return -1; + if (stdin_path) { + if (SELinuxSetFilecon(stdin_path, default_content_context) < 0 && + virStorageFileIsSharedFSType(stdin_path, + VIR_STORAGE_FILE_SHFS_NFS) != 1) + return -1; + } return 0; } diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 3cd5dbc..0dc9f99 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -804,7 +804,8 @@ virStorageFileGetMetadata(const char *path, # endif -int virStorageFileIsSharedFS(const char *path) +int virStorageFileIsSharedFSType(const char *path, + int fstypes) { char *dirpath, *p; struct statfs sb; @@ -853,19 +854,36 @@ int virStorageFileIsSharedFS(const char *path) VIR_DEBUG("Check if path %s with FS magic %lld is shared", path, (long long int)sb.f_type); - if (sb.f_type == NFS_SUPER_MAGIC || - sb.f_type == GFS2_MAGIC || - sb.f_type == OCFS2_SUPER_MAGIC || - sb.f_type == AFS_FS_MAGIC) { + if ((fstypes & VIR_STORAGE_FILE_SHFS_NFS) && + (sb.f_type == NFS_SUPER_MAGIC)) + return 1; + + if ((fstypes & VIR_STORAGE_FILE_SHFS_GFS2) && + (sb.f_type == GFS2_MAGIC)) + return 1; + if ((fstypes & VIR_STORAGE_FILE_SHFS_OCFS) && + (sb.f_type == OCFS2_SUPER_MAGIC)) + return 1; + if ((fstypes & VIR_STORAGE_FILE_SHFS_AFS) && + (sb.f_type == AFS_FS_MAGIC)) return 1; - } return 0; } #else -int virStorageFileIsSharedFS(const char *path ATTRIBUTE_UNUSED) +int virStorageFileIsSharedFSType(const char *path ATTRIBUTE_UNUSED, + int fstypes ATTRIBUTE_UNUSED) { /* XXX implement me :-) */ return 0; } #endif + +int virStorageFileIsSharedFS(const char *path) +{ + return virStorageFileIsSharedFSType(path, + VIR_STORAGE_FILE_SHFS_NFS | + VIR_STORAGE_FILE_SHFS_GFS2 | + VIR_STORAGE_FILE_SHFS_OCFS | + VIR_STORAGE_FILE_SHFS_AFS); +} diff --git a/src/util/storage_file.h b/src/util/storage_file.h index 6853182..ba44111 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -68,6 +68,15 @@ int virStorageFileGetMetadataFromFD(const char *path, int format, virStorageFileMetadata *meta); +enum { + VIR_STORAGE_FILE_SHFS_NFS = (1 << 0), + VIR_STORAGE_FILE_SHFS_GFS2 = (1 << 1), + VIR_STORAGE_FILE_SHFS_OCFS = (1 << 2), + VIR_STORAGE_FILE_SHFS_AFS = (1 << 3), +}; + int virStorageFileIsSharedFS(const char *path); +int virStorageFileIsSharedFSType(const char *path, + int fstypes); #endif /* __VIR_STORAGE_FILE_H__ */ -- 1.7.2.3
On 11/01/2010 11:48 AM, Daniel P. Berrange wrote:
NFS does not support file labelling, so ignore this error for stdin_path when on NFS.
* src/security/security_selinux.c: Ignore failures on labelling stdin_path on NFS * src/util/storage_file.c, src/util/storage_file.h: Refine virStorageFileIsSharedFS() to allow it to check for a specific FS type. --- src/libvirt_private.syms | 1 + src/security/security_selinux.c | 9 ++++++--- src/util/storage_file.c | 32 +++++++++++++++++++++++++------- src/util/storage_file.h | 9 +++++++++ 4 files changed, 41 insertions(+), 10 deletions(-)
-int virStorageFileIsSharedFS(const char *path) +int virStorageFileIsSharedFSType(const char *path, + int fstypes)
Indentation. ACK with that nit fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
NFS in root squash mode may prevent opening disk images to determine backing store. Ignore errors in this scenario. * src/security/security_selinux.c: Ignore open failures on disk images --- src/security/security_selinux.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0612ce3..edeff10 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -482,7 +482,7 @@ SELinuxSetSecurityImageLabel(virSecurityDriverPtr drv, return virDomainDiskDefForeachPath(disk, allowDiskFormatProbing, - false, + true, SELinuxSetSecurityFileLabel, secdef); } -- 1.7.2.3
On 11/01/2010 11:48 AM, Daniel P. Berrange wrote:
NFS in root squash mode may prevent opening disk images to determine backing store. Ignore errors in this scenario.
* src/security/security_selinux.c: Ignore open failures on disk images --- src/security/security_selinux.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0612ce3..edeff10 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -482,7 +482,7 @@ SELinuxSetSecurityImageLabel(virSecurityDriverPtr drv,
return virDomainDiskDefForeachPath(disk, allowDiskFormatProbing, - false, + true, SELinuxSetSecurityFileLabel, secdef); }
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
A couple of places in the text monitor were overwriting the 'ret' variable with a >= 0 value before success was actually determined. So later error paths would not correctly return the -1 value. The drive_add code was not checking for errors like missing command * src/qemu/qemu_monitor_text.c: Misc error handling fixes --- src/qemu/qemu_monitor_text.c | 28 +++++++++++++++++++--------- 1 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index d7e128c..7f15008 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2094,11 +2094,10 @@ int qemuMonitorTextAttachDrive(qemuMonitorPtr mon, } try_command: - ret = virAsprintf(&cmd, "drive_add %s%.2x:%.2x:%.2x %s", - (tryOldSyntax ? "" : "pci_addr="), - controllerAddr->domain, controllerAddr->bus, - controllerAddr->slot, safe_str); - if (ret == -1) { + if (virAsprintf(&cmd, "drive_add %s%.2x:%.2x:%.2x %s", + (tryOldSyntax ? "" : "pci_addr="), + controllerAddr->domain, controllerAddr->bus, + controllerAddr->slot, safe_str) < 0) { virReportOOMError(); goto cleanup; } @@ -2109,6 +2108,12 @@ try_command: goto cleanup; } + if (strstr(reply, "unknown command:")) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("drive hotplug is not supported")); + goto cleanup; + } + if (qemudParseDriveAddReply(reply, driveAddr) < 0) { if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { VIR_FREE(reply); @@ -2360,8 +2365,7 @@ int qemuMonitorTextAddDrive(qemuMonitorPtr mon, /* 'dummy' here is just a placeholder since there is no PCI * address required when attaching drives to a controller */ - ret = virAsprintf(&cmd, "drive_add dummy %s", safe_str); - if (ret == -1) { + if (virAsprintf(&cmd, "drive_add dummy %s", safe_str) < 0) { virReportOOMError(); goto cleanup; } @@ -2372,6 +2376,12 @@ int qemuMonitorTextAddDrive(qemuMonitorPtr mon, goto cleanup; } + if (strstr(reply, "unknown command:")) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("drive hotplug is not supported")); + goto cleanup; + } + ret = 0; cleanup: @@ -2397,8 +2407,8 @@ int qemuMonitorTextSetDrivePassphrase(qemuMonitorPtr mon, return -1; } - ret = virAsprintf(&cmd, "block_passwd %s%s \"%s\"", QEMU_DRIVE_HOST_PREFIX, alias, safe_str); - if (ret == -1) { + if (virAsprintf(&cmd, "block_passwd %s%s \"%s\"", + QEMU_DRIVE_HOST_PREFIX, alias, safe_str) < 0) { virReportOOMError(); goto cleanup; } -- 1.7.2.3
On 11/01/2010 11:48 AM, Daniel P. Berrange wrote:
A couple of places in the text monitor were overwriting the 'ret' variable with a >= 0 value before success was actually determined. So later error paths would not correctly return the -1 value. The drive_add code was not checking for errors like missing command
* src/qemu/qemu_monitor_text.c: Misc error handling fixes --- src/qemu/qemu_monitor_text.c | 28 +++++++++++++++++++--------- 1 files changed, 19 insertions(+), 9 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake