- Devel - libvirt List Archives

[libvirt] [PATCH] build: avoid compile failure on linux kernels older than 2.6.19
by Jim Meyering 17 May '10

17 May '10

This is required for any kernel prior to 2.6.19, since <linux/magic.h> didn't exist back then. Now that file is provided by the kernel-headers package. >From d14ef1669968ffeb65076b007e318934ed99aa61 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 17 May 2010 16:17:08 +0200 Subject: [PATCH] build: avoid compile failure on linux kernels older than 2.6.19 * configure.ac: Check for <linux/magic.h>. * src/util/storage_file.c: Include <linux/magic.h> only if present. Linux kernels prior to 2.6.19 lacked it. [__linux__] (NFS_SUPER_MAGIC): Define if not already defined. --- configure.ac | 2 +- src/util/storage_file.c | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index c187420..ebd2082 100644 --- a/configure.ac +++ b/configure.ac @@ -108,7 +108,7 @@ LIBS=$old_libs dnl Availability of various common headers (non-fatal if missing). AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/syslimits.h \ - termios.h sys/poll.h syslog.h mntent.h net/ethernet.h]) + termios.h sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h]) dnl Where are the XDR functions? dnl If portablexdr is installed, prefer that. diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 5f15a64..a07bedc 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -27,7 +27,9 @@ #include <unistd.h> #include <fcntl.h> #ifdef __linux__ -# include <linux/magic.h> +# if HAVE_LINUX_MAGIC_H +# include <linux/magic.h> +# endif # include <sys/statfs.h> #endif #include "dirname.h" @@ -416,6 +418,9 @@ virStorageFileGetMetadata(const char *path, #ifdef __linux__ +# ifndef NFS_SUPER_MAGIC +# define NFS_SUPER_MAGIC 0x6969 +# endif # ifndef OCFS2_SUPER_MAGIC # define OCFS2_SUPER_MAGIC 0x7461636f # endif -- 1.7.1.250.g7d1e8

3 6

[libvirt] [PATCH] x86ModelHasFeature: avoid NULL-dereference for unmatched CPU "feature"
by Jim Meyering 17 May '10

17 May '10

Here's another fix for a potential NULL-deref. x86cpuidFind can return NULL, yet this caller would dereference that pointer (via x86cpuidMatchMasked) without first checking. >From 9e759e2714b67ea98b18aafb66b5a99ad6361086 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 17 May 2010 14:06:13 +0200 Subject: [PATCH] x86ModelHasFeature: avoid NULL-dereference for unmatched CPU "feature" * src/cpu/cpu_x86.c (x86ModelHasFeature): Do not dereference the pointer returned by x86cpuidFind without first ensuring it is non-NULL. --- src/cpu/cpu_x86.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 633eb69..f7473bf 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -602,31 +602,31 @@ x86ModelMergeFeature(struct x86_model *model, static bool x86ModelHasFeature(struct x86_model *model, const struct x86_feature *feature) { unsigned int i; struct cpuX86cpuid *cpuid; struct cpuX86cpuid *model_cpuid; if (feature == NULL) return false; for (i = 0; i < feature->ncpuid; i++) { cpuid = feature->cpuid + i; model_cpuid = x86cpuidFind(model->cpuid, model->ncpuid, cpuid->function); - if (!x86cpuidMatchMasked(model_cpuid, cpuid)) + if (!model_cpuid || !x86cpuidMatchMasked(model_cpuid, cpuid)) return false; } return true; } static struct x86_model * x86ModelFromCPU(const virCPUDefPtr cpu, const struct x86_map *map, int policy) { struct x86_model *model = NULL; int i; -- 1.7.1.250.g7d1e8

2 2

[libvirt] [PATCH] qemu: Report cmdline output if VM dies early
by Cole Robinson 17 May '10

17 May '10

qemuReadLogOutput early VM death detection is racy and won't always work. Startup then errors when connecting to the VM monitor. This won't report the emulator cmdline output which is typically the most useful diagnostic. Check if the VM has died at the very end of the monitor connection step, and if so, report the cmdline output. See also: https://bugzilla.redhat.com/show_bug.cgi?id=581381 Signed-off-by: Cole Robinson <crobinso(a)redhat.com> --- src/qemu/qemu_driver.c | 59 ++++++++++++++++++++++++++++++++++------------- 1 files changed, 42 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42a653a..c446028 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2029,39 +2029,47 @@ static void qemudFreePtyPath(void *payload, const char *name ATTRIBUTE_UNUSED) VIR_FREE(payload); } +static void +qemuReadLogFD(int logfd, char *buf, int maxlen, int off) +{ + int ret; + char *tmpbuf = buf+off; + + ret = saferead(logfd, tmpbuf, maxlen-off-1); + if (ret < 0) { + ret = 0; + } + + *(tmpbuf+ret) = '\0'; +} + static int qemudWaitForMonitor(struct qemud_driver* driver, virDomainObjPtr vm, off_t pos) { - char buf[4096]; /* Plenty of space to get startup greeting */ + char buf[4096] = "\0"; /* Plenty of space to get startup greeting */ int logfd; int ret = -1; + virHashTablePtr paths = NULL; - if ((logfd = qemudLogReadFD(driver->logDir, vm->def->name, pos)) - < 0) + if ((logfd = qemudLogReadFD(driver->logDir, vm->def->name, pos)) < 0) return -1; - ret = qemudReadLogOutput(vm, logfd, buf, sizeof(buf), - qemudFindCharDevicePTYs, - "console", 30); - if (close(logfd) < 0) { - char ebuf[4096]; - VIR_WARN(_("Unable to close logfile: %s"), - virStrerror(errno, ebuf, sizeof ebuf)); - } - - if (ret < 0) - return -1; + if (qemudReadLogOutput(vm, logfd, buf, sizeof(buf), + qemudFindCharDevicePTYs, + "console", 30) < 0) + goto closelog; VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name); - if (qemuConnectMonitor(driver, vm) < 0) - return -1; + if (qemuConnectMonitor(driver, vm) < 0) { + goto cleanup; + } /* Try to get the pty path mappings again via the monitor. This is much more * reliable if it's available. * Note that the monitor itself can be on a pty, so we still need to try the * log output method. */ - virHashTablePtr paths = virHashCreate(0); + paths = virHashCreate(0); if (paths == NULL) { virReportOOMError(); goto cleanup; @@ -2082,6 +2090,23 @@ cleanup: virHashFree(paths, qemudFreePtyPath); } + if (kill(vm->pid, 0) == -1 && errno == ESRCH) { + /* VM is dead, any other error raised in the interim is probably + * not as important as the qemu cmdline output */ + qemuReadLogFD(logfd, buf, sizeof(buf), strlen(buf)); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("process exited while connecting to monitor: %s"), + buf); + ret = -1; + } + +closelog: + if (close(logfd) < 0) { + char ebuf[4096]; + VIR_WARN(_("Unable to close logfile: %s"), + virStrerror(errno, ebuf, sizeof ebuf)); + } + return ret; } -- 1.7.0.1

2 2

[libvirt] [PATCH] qemu_driver: avoid NULL dereference
by Jim Meyering 17 May '10

17 May '10

The following theoretical possibility of a NULL dereference has been in the code since April 1 (commit 6e41f30efcac08e50b21d9c943d6d27e90555951). It's theoretical, because if that vm = NULL statement is ever executed, the very next one, calling virDomainObjUnlock would dereference that now-NULL "vm". Hence, I think we can conclude the vm = NULL statement is effectively dead code. That conclusion is in line with the "should" in the preceding comment. At first, it seemed like it would deserve an sa_assert. But without the assert, "n_refs" would be unused, (so this first patch is solely FYI -- not proposed) I solved it differently in the 2nd patch. >From 524aec3ebed613f86b64584d2f461f4a18d2e618 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 17 May 2010 12:10:52 +0200 Subject: [PATCH] qemu_driver: avoid NULL dereference * src/qemu/qemu_driver.c (qemudDomainStart): Rather than trying to handle a "can't happen" case, simply sa_assert that it won't happen. --- src/qemu/qemu_driver.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8f69b5a..819ea17 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6489,8 +6489,9 @@ static int qemudDomainStart(virDomainPtr dom) { * We should still have a reference left to vm but * one should check for 0 anyway */ - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + int n_refs = qemuDomainObjEndJob(vm); + sa_assert (n_refs); + virDomainObjUnlock(vm); qemuDriverUnlock(driver); ret = qemudDomainRestore(dom->conn, managed_save); -- 1.7.1.250.g7d1e8 >From f88969b986a1c88985671c9d6fa9cb1dc449ed74 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Mon, 17 May 2010 12:10:52 +0200 Subject: [PATCH] qemu_driver: avoid NULL dereference * src/qemu/qemu_driver.c (qemudDomainStart): After setting vm to NULL, goto cleanup, rather than dereferencing the NULL pointer. --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8f69b5a..3559e36 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6489,8 +6489,11 @@ static int qemudDomainStart(virDomainPtr dom) { * We should still have a reference left to vm but * one should check for 0 anyway */ - if (qemuDomainObjEndJob(vm) == 0) + if (qemuDomainObjEndJob(vm) = 0) { vm = NULL; + goto cleanup; + } + virDomainObjUnlock(vm); qemuDriverUnlock(driver); ret = qemudDomainRestore(dom->conn, managed_save); -- 1.7.1.250.g7d1e8

3 4

[libvirt] [PATCH] Don't reset user/group/security label on shared filesystems during migrate
by Daniel P. Berrange 17 May '10

17 May '10

When QEMU runs with its disk on NFS, and as a non-root user, the disk is chownd to that non-root user. When migration completes the last step is shutting down the QEMU on the source host. THis normally resets user/group/security label. This is bad when the VM was just migrated because the file is still in use on the dest host. It is thus neccessary to skip the reset step for any files found to be on a shared filesystem * src/libvirt_private.syms: Export virStorageFileIsSharedFS * src/util/storage_file.c, src/util/storage_file.h: Add a new method virStorageFileIsSharedFS() to determine if a file is on a shared filesystem (NFS, GFS, OCFS2, etc) * src/qemu/qemu_driver.c: Tell security driver not to reset disk labels on migration completion * src/qemu/qemu_security_dac.c, src/qemu/qemu_security_stacked.c, src/security/security_selinux.c, src/security/security_driver.h, src/security/security_apparmor.c: Add ability to skip disk restore step for files on shared filesystems. --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 34 ++++++++++++++----------- src/qemu/qemu_security_dac.c | 40 +++++++++++++++++++++++++---- src/qemu/qemu_security_stacked.c | 7 +++-- src/security/security_apparmor.c | 3 +- src/security/security_driver.h | 3 +- src/security/security_selinux.c | 38 +++++++++++++++++++++++++---- src/util/storage_file.c | 50 ++++++++++++++++++++++++++++++++++++++ src/util/storage_file.h | 2 + 9 files changed, 147 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5713b2c..7d7ee14 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -617,6 +617,7 @@ virStorageFileFormatTypeToString; virStorageFileFormatTypeFromString; virStorageFileGetMetadata; virStorageFileGetMetadataFromFD; +virStorageFileIsSharedFS; # threads.h virMutexInit; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b478464..702a652 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -174,7 +174,8 @@ static int qemudStartVMDaemon(virConnectPtr conn, int stdin_fd); static void qemudShutdownVMDaemon(struct qemud_driver *driver, - virDomainObjPtr vm); + virDomainObjPtr vm, + int migrated); static int qemudDomainGetMaxVcpus(virDomainPtr dom); @@ -1105,7 +1106,7 @@ qemuHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, VIR_DOMAIN_EVENT_STOPPED_FAILED : VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - qemudShutdownVMDaemon(driver, vm); + qemudShutdownVMDaemon(driver, vm, 0); if (!vm->persistent) virDomainRemoveInactive(&driver->domains, vm); else @@ -1636,7 +1637,7 @@ error: /* We can't get the monitor back, so must kill the VM * to remove danger of it ending up running twice if * user tries to start it again later */ - qemudShutdownVMDaemon(driver, obj); + qemudShutdownVMDaemon(driver, obj, 0); if (!obj->persistent) virDomainRemoveInactive(&driver->domains, obj); else @@ -4009,7 +4010,7 @@ cleanup: if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityAllLabel) - driver->securityDriver->domainRestoreSecurityAllLabel(vm); + driver->securityDriver->domainRestoreSecurityAllLabel(vm, 0); if (driver->securityDriver && driver->securityDriver->domainReleaseSecurityLabel) driver->securityDriver->domainReleaseSecurityLabel(vm); @@ -4032,7 +4033,7 @@ cleanup: abort: /* We jump here if we failed to initialize the now running VM * killing it off and pretend we never started it */ - qemudShutdownVMDaemon(driver, vm); + qemudShutdownVMDaemon(driver, vm, 0); if (logfile != -1) close(logfile); @@ -4042,7 +4043,8 @@ abort: static void qemudShutdownVMDaemon(struct qemud_driver *driver, - virDomainObjPtr vm) { + virDomainObjPtr vm, + int migrated) { int ret; int retries = 0; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -4053,7 +4055,7 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, if (!virDomainObjIsActive(vm)) return; - VIR_DEBUG("Shutting down VM '%s'", vm->def->name); + VIR_DEBUG("Shutting down VM '%s' migrated=%d", vm->def->name, migrated); /* This method is routinely used in clean up paths. Disable error * reporting so we don't squash a legit error. */ @@ -4111,7 +4113,7 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, /* Reset Security Labels */ if (driver->securityDriver && driver->securityDriver->domainRestoreSecurityAllLabel) - driver->securityDriver->domainRestoreSecurityAllLabel(vm); + driver->securityDriver->domainRestoreSecurityAllLabel(vm, migrated); if (driver->securityDriver && driver->securityDriver->domainReleaseSecurityLabel) driver->securityDriver->domainReleaseSecurityLabel(vm); @@ -4835,7 +4837,7 @@ static int qemudDomainDestroy(virDomainPtr dom) { goto endjob; } - qemudShutdownVMDaemon(driver, vm); + qemudShutdownVMDaemon(driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); @@ -5546,7 +5548,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, ret = 0; /* Shut it down */ - qemudShutdownVMDaemon(driver, vm); + qemudShutdownVMDaemon(driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SAVED); @@ -5856,7 +5858,7 @@ static int qemudDomainCoreDump(virDomainPtr dom, endjob: if ((ret == 0) && (flags & VIR_DUMP_CRASH)) { - qemudShutdownVMDaemon(driver, vm); + qemudShutdownVMDaemon(driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); @@ -10365,7 +10367,7 @@ qemudDomainMigratePrepareTunnel(virConnectPtr dconn, qemust = qemuStreamMigOpen(st, unixfile); if (qemust == NULL) { - qemudShutdownVMDaemon(driver, vm); + qemudShutdownVMDaemon(driver, vm, 0); if (!vm->persistent) { if (qemuDomainObjEndJob(vm) > 0) virDomainRemoveInactive(&driver->domains, vm); @@ -11093,7 +11095,9 @@ qemudDomainMigratePerform (virDomainPtr dom, } /* Clean up the source domain. */ - qemudShutdownVMDaemon(driver, vm); + fprintf(stderr, "******************* MIG \n"); + qemudShutdownVMDaemon(driver, vm, 1); + fprintf(stderr, "******************* YEEHAAA\n"); resume = 0; event = virDomainEventNewFromObj(vm, @@ -11235,7 +11239,7 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn, } virDomainSaveStatus(driver->caps, driver->stateDir, vm); } else { - qemudShutdownVMDaemon(driver, vm); + qemudShutdownVMDaemon(driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); @@ -12081,7 +12085,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, */ if (virDomainObjIsActive(vm)) { - qemudShutdownVMDaemon(driver, vm); + qemudShutdownVMDaemon(driver, vm, 0); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c index e408dbf..2d42ce2 100644 --- a/src/qemu/qemu_security_dac.c +++ b/src/qemu/qemu_security_dac.c @@ -142,8 +142,9 @@ qemuSecurityDACSetSecurityImageLabel(virDomainObjPtr vm ATTRIBUTE_UNUSED, static int -qemuSecurityDACRestoreSecurityImageLabel(virDomainObjPtr vm ATTRIBUTE_UNUSED, - virDomainDiskDefPtr disk) +qemuSecurityDACRestoreSecurityImageLabelInt(virDomainObjPtr vm ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk, + int migrated) { if (!driver->privileged || !driver->dynamicOwnership) return 0; @@ -162,11 +163,35 @@ qemuSecurityDACRestoreSecurityImageLabel(virDomainObjPtr vm ATTRIBUTE_UNUSED, if (!disk->src) return 0; + /* If we have a shared FS & doing migrated, we must not + * change ownership, because that kills access on the + * destination host which is sub-optimal for the guest + * VM's I/O attempts :-) + */ + if (migrated) { + int rc = virStorageFileIsSharedFS(disk->src); + if (rc < 0) + return -1; + if (rc == 1) { + VIR_DEBUG("Skipping image label restore on %s because FS is shared", + disk->src); + return 0; + } + } + return qemuSecurityDACRestoreSecurityFileLabel(disk->src); } static int +qemuSecurityDACRestoreSecurityImageLabel(virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + return qemuSecurityDACRestoreSecurityImageLabelInt(vm, disk, 0); +} + + +static int qemuSecurityDACSetSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED, const char *file, void *opaque ATTRIBUTE_UNUSED) @@ -306,7 +331,8 @@ done: static int -qemuSecurityDACRestoreSecurityAllLabel(virDomainObjPtr vm) +qemuSecurityDACRestoreSecurityAllLabel(virDomainObjPtr vm, + int migrated) { int i; int rc = 0; @@ -314,7 +340,8 @@ qemuSecurityDACRestoreSecurityAllLabel(virDomainObjPtr vm) if (!driver->privileged || !driver->dynamicOwnership) return 0; - VIR_DEBUG("Restoring security label on %s", vm->def->name); + VIR_DEBUG("Restoring security label on %s migrated=%d", + vm->def->name, migrated); for (i = 0 ; i < vm->def->nhostdevs ; i++) { if (qemuSecurityDACRestoreSecurityHostdevLabel(vm, @@ -322,8 +349,9 @@ qemuSecurityDACRestoreSecurityAllLabel(virDomainObjPtr vm) rc = -1; } for (i = 0 ; i < vm->def->ndisks ; i++) { - if (qemuSecurityDACRestoreSecurityImageLabel(vm, - vm->def->disks[i]) < 0) + if (qemuSecurityDACRestoreSecurityImageLabelInt(vm, + vm->def->disks[i], + migrated) < 0) rc = -1; } diff --git a/src/qemu/qemu_security_stacked.c b/src/qemu/qemu_security_stacked.c index c0258ce..04c1f10 100644 --- a/src/qemu/qemu_security_stacked.c +++ b/src/qemu/qemu_security_stacked.c @@ -215,18 +215,19 @@ qemuSecurityStackedSetSecurityAllLabel(virDomainObjPtr vm) static int -qemuSecurityStackedRestoreSecurityAllLabel(virDomainObjPtr vm) +qemuSecurityStackedRestoreSecurityAllLabel(virDomainObjPtr vm, + int migrated) { int rc = 0; if (driver->securitySecondaryDriver && driver->securitySecondaryDriver->domainRestoreSecurityAllLabel && - driver->securitySecondaryDriver->domainRestoreSecurityAllLabel(vm) < 0) + driver->securitySecondaryDriver->domainRestoreSecurityAllLabel(vm, migrated) < 0) rc = -1; if (driver->securityPrimaryDriver && driver->securityPrimaryDriver->domainRestoreSecurityAllLabel && - driver->securityPrimaryDriver->domainRestoreSecurityAllLabel(vm) < 0) + driver->securityPrimaryDriver->domainRestoreSecurityAllLabel(vm, migrated) < 0) rc = -1; return rc; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index c0c91cc..87f8a1b 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -444,7 +444,8 @@ AppArmorReleaseSecurityLabel(virDomainObjPtr vm) static int -AppArmorRestoreSecurityAllLabel(virDomainObjPtr vm) +AppArmorRestoreSecurityAllLabel(virDomainObjPtr vm, + int migrated ATTRIBUTE_UNUSED) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int rc = 0; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 3de234a..39edc6d 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -46,7 +46,8 @@ typedef int (*virSecurityDomainGenLabel) (virDomainObjPtr sec); typedef int (*virSecurityDomainReserveLabel) (virDomainObjPtr sec); typedef int (*virSecurityDomainReleaseLabel) (virDomainObjPtr sec); typedef int (*virSecurityDomainSetAllLabel) (virDomainObjPtr sec); -typedef int (*virSecurityDomainRestoreAllLabel) (virDomainObjPtr vm); +typedef int (*virSecurityDomainRestoreAllLabel) (virDomainObjPtr vm, + int migrated); typedef int (*virSecurityDomainGetProcessLabel) (virDomainObjPtr vm, virSecurityLabelPtr sec); typedef int (*virSecurityDomainSetProcessLabel) (virSecurityDriverPtr drv, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 1aabb20..47534df 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -385,8 +385,9 @@ err: } static int -SELinuxRestoreSecurityImageLabel(virDomainObjPtr vm, - virDomainDiskDefPtr disk) +SELinuxRestoreSecurityImageLabelInt(virDomainObjPtr vm, + virDomainDiskDefPtr disk, + int migrated) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; @@ -407,9 +408,34 @@ SELinuxRestoreSecurityImageLabel(virDomainObjPtr vm, if (!disk->src) return 0; + /* If we have a shared FS & doing migrated, we must not + * change ownership, because that kills access on the + * destination host which is sub-optimal for the guest + * VM's I/O attempts :-) + */ + if (migrated) { + int rc = virStorageFileIsSharedFS(disk->src); + if (rc < 0) + return -1; + if (rc == 1) { + VIR_DEBUG("Skipping image label restore on %s because FS is shared", + disk->src); + return 0; + } + } + return SELinuxRestoreSecurityFileLabel(disk->src); } + +static int +SELinuxRestoreSecurityImageLabel(virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + return SELinuxRestoreSecurityImageLabelInt(vm, disk, 0); +} + + static int SELinuxSetSecurityImageLabel(virDomainObjPtr vm, virDomainDiskDefPtr disk) @@ -603,7 +629,8 @@ done: } static int -SELinuxRestoreSecurityAllLabel(virDomainObjPtr vm) +SELinuxRestoreSecurityAllLabel(virDomainObjPtr vm, + int migrated ATTRIBUTE_UNUSED) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int i; @@ -619,8 +646,9 @@ SELinuxRestoreSecurityAllLabel(virDomainObjPtr vm) rc = -1; } for (i = 0 ; i < vm->def->ndisks ; i++) { - if (SELinuxRestoreSecurityImageLabel(vm, - vm->def->disks[i]) < 0) + if (SELinuxRestoreSecurityImageLabelInt(vm, + vm->def->disks[i], + migrated) < 0) rc = -1; } diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 76ebb98..d4aad86 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -26,9 +26,14 @@ #include <unistd.h> #include <fcntl.h> +#ifdef __linux__ +#include <linux/magic.h> +#include <sys/statfs.h> +#endif #include "dirname.h" #include "memory.h" #include "virterror_internal.h" +#include "logging.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -407,3 +412,48 @@ virStorageFileGetMetadata(const char *path, return ret; } + + +#ifdef __linux__ + +#ifndef OCFS2_SUPER_MAGIC +#define OCFS2_SUPER_MAGIC 0x7461636f +#endif +#ifndef GFS2_MAGIC +#define GFS2_MAGIC 0x01161970 +#endif +#ifndef AFS_FS_MAGIC +#define AFS_FS_MAGIC 0x6B414653 +#endif + + +int virStorageFileIsSharedFS(const char *path) +{ + struct statfs sb; + + if (statfs(path, &sb) < 0) { + virReportSystemError(errno, + _("cannot determine filesystem for '%s'"), + path); + return -1; + } + + 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) { + return 1; + } + + return 0; +} +#else +int virStorageFileIsSharedFS(const char *path ATTRIBUTE_UNUSED) +{ + /* XXX implement me :-) */ + return 0; +} +#endif diff --git a/src/util/storage_file.h b/src/util/storage_file.h index c761dc6..58533ee 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -61,4 +61,6 @@ int virStorageFileGetMetadataFromFD(const char *path, int fd, virStorageFileMetadata *meta); +int virStorageFileIsSharedFS(const char *path); + #endif /* __VIR_STORAGE_FILE_H__ */ -- 1.6.6.1

4 5

[libvirt] [PATCH] nwfilter: Add missing driver lock in qemu driver
by Stefan Berger 17 May '10

17 May '10

This adds a missing driver lock in the qemu driver to protect the list of domains. Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com> --- src/qemu/qemu_driver.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -11869,11 +11869,15 @@ static virStateDriver qemuStateDriver = }; static int -qemudVMFilterRebuild(virConnectPtr conn, +qemudVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED, virHashIterator iter, void *data) { - (void)conn; + struct qemud_driver *driver = qemu_driver; + + qemuDriverLock(driver); virHashForEach(qemu_driver->domains.objs, iter, data); + qemuDriverUnlock(driver); + return 0; }

3 3

[libvirt] [PATCH] qemudDomainSetVcpus: avoid NULL-deref
by Jim Meyering 15 May '10

15 May '10

Here's a patch for a coverity-spotted bug: >From 12160fa54bc948e5de3fecff6a9552995e9595b0 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering(a)redhat.com> Date: Fri, 14 May 2010 12:38:43 +0200 Subject: [PATCH] qemudDomainSetVcpus: avoid NULL-deref * src/qemu/qemu_driver.c (qemudDomainSetVcpus): Avoid NULL-deref upon unknown UUID. Call qemuDomainObjBeginJob(vm) only after ensuring that vm != NULL, not before. This potential NULL-deref was introduced by commit 2c555d87b0041e0d1ec4742386d2161d1b2f0600. --- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bb1079e..cbddb96 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5410,9 +5410,6 @@ static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); - if (qemuDomainObjBeginJob(vm) < 0) - goto cleanup; - if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); @@ -5421,6 +5418,9 @@ static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { goto endjob; } + if (qemuDomainObjBeginJob(vm) < 0) + goto cleanup; + if (!virDomainObjIsActive(vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); -- 1.7.1.460.gf3c4c

2 2

[libvirt] [PATCH] Fix a misuse of virAsprintf in qemudDomainMemoryPeek
by Ryota Ozaki 14 May '10

14 May '10

The code specifies driver->cacheDir as the format string, but it usually doesn't contain '%s', so the subsequent argument, "/qemu.mem.XXXXXX", is always ignored. The patch fixes the misuse. --- src/qemu/qemu_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bb1079e..843f827 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9033,7 +9033,7 @@ qemudDomainMemoryPeek (virDomainPtr dom, goto endjob; } - if (virAsprintf(&tmp, driver->cacheDir, "/qemu.mem.XXXXXX") < 0) { + if (virAsprintf(&tmp, "%s/qemu.mem.XXXXXX", driver->cacheDir) < 0) { virReportOOMError(); goto endjob; } -- 1.6.5.2

2 2

[libvirt] How many domains can be created and run in parallel in a Host.
by Kumar L Srikanth-B22348 14 May '10

14 May '10

Hi, I have a general doubt regarding Virtual Domains in libvirt. How many Virtual Domains can be created and run in parallel [at max] in a host machine using libvirt. Regards, Srikanth.

2 1

[libvirt] [PATCH] Query block allocation extent from QEMU monitor
by Daniel P. Berrange 14 May '10

14 May '10

The virDomainGetBlockInfo API allows query physical block extent and allocated block extent. These are normally the same value unless storing a special format like qcow2 inside a block device. In this scenario we can query QEMU to get the actual allocated extent. * src/qemu/qemu_driver.c: Fill in block aloction extent when VM is running * src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h, src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h, src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h: Add API to query the highest block extent via info blockstats --- src/qemu/qemu_driver.c | 32 +++++++++++--- src/qemu/qemu_monitor.c | 16 +++++++ src/qemu/qemu_monitor.h | 4 ++ src/qemu/qemu_monitor_json.c | 97 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + src/qemu/qemu_monitor_text.c | 12 +++++ src/qemu/qemu_monitor_text.h | 4 +- 7 files changed, 160 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5089129..4251a66 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9651,6 +9651,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, int fd = -1; off_t end; virStorageFileMetadata meta; + virDomainDiskDefPtr disk = NULL; struct stat sb; int i; @@ -9677,19 +9678,17 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, for (i = 0 ; i < vm->def->ndisks ; i++) { if (vm->def->disks[i]->src != NULL && STREQ (vm->def->disks[i]->src, path)) { - ret = 0; + disk = vm->def->disks[i]; break; } } - if (ret != 0) { + if (!disk) { qemuReportError(VIR_ERR_INVALID_ARG, _("invalid path %s not assigned to domain"), path); goto cleanup; } - ret = -1; - /* The path is correct, now try to open it and get its size. */ fd = open (path, O_RDONLY); if (fd == -1) { @@ -9740,11 +9739,30 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, if (meta.capacity) info->capacity = meta.capacity; - /* XXX allocation will need to be pulled from QEMU for - * the qcow inside LVM case */ + /* Set default value .. */ info->allocation = info->physical; - ret = 0; + /* ..but if guest is running & not using raw + disk format and on a block device, then query + highest allocated extent from QEMU */ + if (virDomainObjIsActive(vm) && + meta.format != VIR_STORAGE_FILE_RAW && + S_ISBLK(sb.st_mode)) { + qemuDomainObjPrivatePtr priv = vm->privateData; + if (qemuDomainObjBeginJob(vm) < 0) + goto cleanup; + + qemuDomainObjEnterMonitor(vm); + ret = qemuMonitorGetBlockExtent(priv->mon, + disk->info.alias, + &info->allocation); + qemuDomainObjExitMonitor(vm); + + if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; + } else { + ret = 0; + } cleanup: if (fd != -1) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f77ec44..4a77f39 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1000,6 +1000,22 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, return ret; } +int qemuMonitorGetBlockExtent(qemuMonitorPtr mon, + const char *devname, + unsigned long long *extent) +{ + int ret; + DEBUG("mon=%p, fd=%d, devname=%p", + mon, mon->fd, devname); + + if (mon->json) + ret = qemuMonitorJSONGetBlockExtent(mon, devname, extent); + else + ret = qemuMonitorTextGetBlockExtent(mon, devname, extent); + + return ret; +} + int qemuMonitorSetVNCPassword(qemuMonitorPtr mon, const char *password) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7b1589e..adfb3bc 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -185,6 +185,10 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon, long long *wr_bytes, long long *errs); +int qemuMonitorGetBlockExtent(qemuMonitorPtr mon, + const char *devname, + unsigned long long *extent); + int qemuMonitorSetVNCPassword(qemuMonitorPtr mon, const char *password); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6d8f328..a15609c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1186,6 +1186,103 @@ cleanup: } +int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, + const char *devname, + unsigned long long *extent) +{ + int ret; + int i; + int found = 0; + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats", + NULL); + virJSONValuePtr reply = NULL; + virJSONValuePtr devices; + + *extent = 0; + + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &reply); + + if (ret == 0) { + ret = qemuMonitorJSONCheckError(cmd, reply); + if (ret < 0) + goto cleanup; + } + ret = -1; + + devices = virJSONValueObjectGet(reply, "return"); + if (!devices || devices->type != VIR_JSON_TYPE_ARRAY) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats reply was missing device list")); + goto cleanup; + } + + for (i = 0 ; i < virJSONValueArraySize(devices) ; i++) { + virJSONValuePtr dev = virJSONValueArrayGet(devices, i); + virJSONValuePtr stats; + virJSONValuePtr parent; + const char *thisdev; + if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats device entry was not in expected format")); + goto cleanup; + } + + if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats device entry was not in expected format")); + goto cleanup; + } + + /* New QEMU has separate names for host & guest side of the disk + * and libvirt gives the host side a 'drive-' prefix. The passed + * in devname is the guest side though + */ + if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX)) + thisdev += strlen(QEMU_DRIVE_HOST_PREFIX); + + if (STRNEQ(thisdev, devname)) + continue; + + found = 1; + if ((parent = virJSONValueObjectGet(dev, "parent")) == NULL || + parent->type != VIR_JSON_TYPE_OBJECT) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats parent entry was not in expected format")); + goto cleanup; + } + + if ((stats = virJSONValueObjectGet(parent, "stats")) == NULL || + stats->type != VIR_JSON_TYPE_OBJECT) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("blockstats stats entry was not in expected format")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberUlong(stats, "wr_highest_offset", extent) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot read %s statistic"), + "wr_highest_offset"); + goto cleanup; + } + } + + if (!found) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find statistics for device '%s'"), devname); + goto cleanup; + } + ret = 0; + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONSetVNCPassword(qemuMonitorPtr mon, const char *password) { diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 26fc865..14597f4 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -56,6 +56,9 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon, long long *wr_req, long long *wr_bytes, long long *errs); +int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon, + const char *devname, + unsigned long long *extent); int qemuMonitorJSONSetVNCPassword(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index d725d6d..19038d1 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -711,6 +711,18 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon, } +int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + const char *devname ATTRIBUTE_UNUSED, + unsigned long long *extent) +{ + /* Not supported in text monitor, but we don't want to + * cause an error in callers in this scenario, just + * fallback to marking the data unavailable */ + *extent = 0; + return 0; +} + + static int qemuMonitorSendVNCPassphrase(qemuMonitorPtr mon ATTRIBUTE_UNUSED, qemuMonitorMessagePtr msg, diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 2a62c7e..6fb7d7a 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -55,7 +55,9 @@ int qemuMonitorTextGetBlockStatsInfo(qemuMonitorPtr mon, long long *wr_req, long long *wr_bytes, long long *errs); - +int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon, + const char *devname, + unsigned long long *extent); int qemuMonitorTextSetVNCPassword(qemuMonitorPtr mon, const char *password); -- 1.6.6.1

2 2