- Devel - libvirt List Archives

[libvirt] [PATCH v3 1/2] qemu: Resolve data loss and data corruption of domain restoring.
by Osier Yang 07 Apr '11

07 Apr '11

Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss. (This problem exists for "virsh managedsave dom; virsh start dom"). And keeping the saved state will cause data corruption if the user modified his disks and restore the domain second time from the saved state. (Problem exists for "virsh save dom; virsh restore dom"). The fix is to: * Don't unlink()s the managed saved state if the restoring fails. * Remove the saved state if restoring succeeded. --- src/qemu/qemu_driver.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..a618df4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3171,6 +3171,9 @@ qemuDomainRestore(virConnectPtr conn, vm = NULL; } + if ((ret == 0) && (unlink(path) < 0)) + VIR_WARN("Failed to remove the saved state %s", path); + cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); @@ -3423,18 +3426,22 @@ static int qemudDomainObjStart(virConnectPtr conn, /* * If there is a managed saved state restore it instead of starting - * from scratch. In any case the old state is removed. + * from scratch. */ managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save) && (virFileExists(managed_save))) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save); - if (unlink(managed_save) < 0) { - VIR_WARN("Failed to remove the managed state %s", managed_save); + if (ret == 0) { + if (unlink(managed_save) < 0) + VIR_WARN("Failed to remove the managed state %s", managed_save); + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restore from the managed state %s"), + managed_save); } - if (ret == 0) - goto cleanup; + goto cleanup; } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, -- 1.7.4

2 6

[libvirt] [PATCH v4] qemu: Unlink the managed state only if restoring succeeded
by Osier Yang 07 Apr '11

07 Apr '11

Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss. (This problem exists for "virsh managedsave dom; virsh start dom"). The fix is to: * Don't unlink()s the managed saved state if the restoring fails. * Add doc for restoring in virsh mannual to tell user it's not encourage to reuse a saved state file. --- src/qemu/qemu_driver.c | 8 +++----- tools/virsh.pod | 6 +++++- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..fdd58c7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3423,18 +3423,16 @@ static int qemudDomainObjStart(virConnectPtr conn, /* * If there is a managed saved state restore it instead of starting - * from scratch. In any case the old state is removed. + * from scratch. The old state is removed once the restoring succeeded. */ managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save) && (virFileExists(managed_save))) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save); - if (unlink(managed_save) < 0) { + if ((ret == 0) && (unlink(managed_save) < 0)) VIR_WARN("Failed to remove the managed state %s", managed_save); - } - if (ret == 0) - goto cleanup; + goto cleanup; } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, diff --git a/tools/virsh.pod b/tools/virsh.pod index f4bd294..6319373 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -546,7 +546,11 @@ I<on_reboot> parameter in the domain's XML definition. =item B<restore> I<state-file> -Restores a domain from an B<virsh save> state file. See I<save> for more info. +Restores a domain from an B<virsh save> state file. See I<save> for more info. + +B<Note>: To avoid corrupting file system contents within the domain, you +should not reuse the saved state file to B<restore> unless you are convinced +with reverting the domain to the previous state. =item B<save> I<domain-id> I<state-file> -- 1.7.4

1 0

[libvirt] the storage API requires capacity even using backingStore
by UbuntuKids China 07 Apr '11

07 Apr '11

Hi, I am using the storage API to create a new qemu image volume which has backingStore. It requires capacity to be set and uses the specified capacity as the size of the new volume. This causes issues if the specified capacity is not the same as the backingStore file's. It's not easy to get the capacity of the backing file in some cases. And qemu-img/kvm-img doesn't require a size option if it uses backing file. Is there any thing I could do to avoid this case? Or it needs a patch? Thanks a lot. BR UbuntuKids -- Ubuntu For Kids in China

1 0

[libvirt] [PATCH] qemu: Do not unlink managedsave image if restoring fails.
by Osier Yang 07 Apr '11

07 Apr '11

Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss. However, I'm not sure if it's the very correct way to fix it, if restoring fails, and we didn't remove the image, it will trys to restore from the image again next time, if that's not the user expected (e.g. the user made quite many changes on the guest), then it's a new problem. --- src/qemu/qemu_driver.c | 12 +++++++----- 1 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..22c29e4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3423,18 +3423,20 @@ static int qemudDomainObjStart(virConnectPtr conn, /* * If there is a managed saved state restore it instead of starting - * from scratch. In any case the old state is removed. + * from scratch. */ managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save) && (virFileExists(managed_save))) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save); - if (unlink(managed_save) < 0) { - VIR_WARN("Failed to remove the managed state %s", managed_save); - } + if (ret == 0) { + if (unlink(managed_save) < 0) + VIR_WARN("Failed to remove the managed state %s", managed_save); - if (ret == 0) goto cleanup; + } else { + VIR_WARN("Failed to restore from the managed state %s", managed_save); + } } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, -- 1.7.4

5 15

[libvirt] [PATCH] build: avoid compiler warning on cygwin
by Eric Blake 07 Apr '11

07 Apr '11

In file included from util/threads.c:31: util/threads-pthread.c: In function 'virThreadSelfID': util/threads-pthread.c:214: warning: cast from function call of type 'pthread_t' to non-matching type 'int' [-Wbad-function-cast] * src/util/threads-pthread.c (virThreadSelfID) [!SYS_gettid]: Add intermediate cast to shut up gcc. --- Pushing under the build-breaker rule. src/util/threads-pthread.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/threads-pthread.c b/src/util/threads-pthread.c index f812045..898c4d4 100644 --- a/src/util/threads-pthread.c +++ b/src/util/threads-pthread.c @@ -1,7 +1,7 @@ /* * threads-pthread.c: basic thread synchronization primitives * - * Copyright (C) 2009-2010 Red Hat, Inc. + * Copyright (C) 2009-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -211,7 +211,7 @@ int virThreadSelfID(void) tid = syscall(SYS_gettid); return (int)tid; #else - return (int)pthread_self(); + return (int)(void *)pthread_self(); #endif } -- 1.7.4

1 0

[libvirt] [PATCH] Fix build for older gcc
by Jim Fehlig 06 Apr '11

06 Apr '11

With gcc 4.3.4 I'm seeing the following warning failure cc1: warnings being treated as errors cc1: error: -funit-at-a-time is required for inlining of functions that are only called once [-Wdisabled-optimization] Add -funit-at-a-time to WARN_CFLAGS. --- m4/virt-compile-warnings.m4 | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 819da8f..8df6f9c 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -112,6 +112,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ gl_WARN_ADD([-fexceptions]) gl_WARN_ADD([-fasynchronous-unwind-tables]) gl_WARN_ADD([-fdiagnostics-show-option]) + gl_WARN_ADD([-funit-at-a-time]) if test "$enable_compile_warnings" = "error" then -- 1.7.3.1

2 2

[libvirt] [PATCH 1/1] Change locking for udev monitor and callbacks
by Serge Hallyn 06 Apr '11

06 Apr '11

We're seeing bugs apparently resulting from thread unsafety of libpciaccess, such as https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/726099 To prevent those, as suggested by danpb on irc, move the nodeDeviceLock(driverState) higher into the callers. In particular: udevDeviceMonitorStartup should hold the lock while calling udevEnumerateDevices(), and udevEventHandleCallback should hold it over its entire execution. It's not clear to me whether it is ok to hold the nodeDeviceLock while taking the virNodeDeviceObjLock(dev) on a device. If not, then the lock will need to be dropped around the calling of udevSetupSystemDev(), and udevAddOneDevice() may not actually be safe to call from higher layers with the driverstate lock held. libvirt 0.8.8 with this patch on it seems to work fine for me. Assuming it looks ok and I haven't done anything obviously dumb, I'll ask the bug submitters to try this patch. Signed-off-by: Serge Hallyn <serge.hallyn(a)ubuntu.com> --- src/node_device/node_device_udev.c | 21 +++++++++------------ 1 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 372f1d1..2139ef3 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1202,7 +1202,6 @@ static int udevRemoveOneDevice(struct udev_device *device) int ret = 0; name = udev_device_get_syspath(device); - nodeDeviceLock(driverState); dev = virNodeDeviceFindBySysfsPath(&driverState->devs, name); if (dev != NULL) { @@ -1214,7 +1213,6 @@ static int udevRemoveOneDevice(struct udev_device *device) name); ret = -1; } - nodeDeviceUnlock(driverState); return ret; } @@ -1316,9 +1314,7 @@ static int udevAddOneDevice(struct udev_device *device) /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - nodeDeviceLock(driverState); dev = virNodeDeviceAssignDef(&driverState->devs, def); - nodeDeviceUnlock(driverState); if (dev == NULL) { VIR_ERROR(_("Failed to create device for '%s'"), def->name); @@ -1442,6 +1438,7 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, const char *action = NULL; int udev_fd = -1; + nodeDeviceLock(driverState); udev_fd = udev_monitor_get_fd(udev_monitor); if (fd != udev_fd) { VIR_ERROR(_("File descriptor returned by udev %d does not " @@ -1470,6 +1467,7 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED, out: udev_device_unref(device); + nodeDeviceUnlock(driverState); return; } @@ -1647,10 +1645,9 @@ static int udevDeviceMonitorStartup(int privileged) priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"); if (priv->udev_monitor == NULL) { VIR_FREE(priv); - nodeDeviceUnlock(driverState); VIR_ERROR0(_("udev_monitor_new_from_netlink returned NULL")); ret = -1; - goto out; + goto out_unlock; } udev_monitor_enable_receiving(priv->udev_monitor); @@ -1670,26 +1667,26 @@ static int udevDeviceMonitorStartup(int privileged) VIR_EVENT_HANDLE_READABLE, udevEventHandleCallback, NULL, NULL); if (priv->watch == -1) { - nodeDeviceUnlock(driverState); ret = -1; - goto out; + goto out_unlock; } - nodeDeviceUnlock(driverState); - /* Create a fictional 'computer' device to root the device tree. */ if (udevSetupSystemDev() != 0) { ret = -1; - goto out; + goto out_unlock; } /* Populate with known devices */ if (udevEnumerateDevices(udev) != 0) { ret = -1; - goto out; + goto out_unlock; } +out_unlock: + nodeDeviceUnlock(driverState); + out: if (ret == -1) { udevDeviceMonitorShutdown(); -- 1.7.4.1

3 3

[libvirt] [PATCH] qemu: Always reserves slot 0x02 for primary VGA.
by Osier Yang 06 Apr '11

06 Apr '11

To address https://bugzilla.redhat.com/show_bug.cgi?id=692355 This fix is to reserve slot 0x02 for primary VGA even if there is no "video" specified in domain XML to avoid the problem. --- src/qemu/qemu_command.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3d25ba4..9b93d5e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -947,6 +947,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) { int i; bool reservedIDE = false; + bool reservedVGA = false; /* Host bridge */ if (qemuDomainPCIAddressReserveSlot(addrs, 0) < 0) @@ -966,7 +967,7 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) _("Primary IDE controller must have PCI address 0:0:1.1")); goto error; } - /* If TYPE==PCI, then then qemuCollectPCIAddress() function + /* If TYPE==PCI, then qemuCollectPCIAddress() function * has already reserved the address, so we must skip */ reservedIDE = true; } else { @@ -997,16 +998,22 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) _("Primary video card must have PCI address 0:0:2.0")); goto error; } + /* If TYPE==PCI, then qemuCollectPCIAddress() function + * has already reserved the address, so we must skip */ + reservedVGA = true; } else { def->videos[0]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; def->videos[0]->info.addr.pci.domain = 0; def->videos[0]->info.addr.pci.bus = 0; def->videos[0]->info.addr.pci.slot = 2; def->videos[0]->info.addr.pci.function = 0; - if (qemuDomainPCIAddressReserveSlot(addrs, 2) < 0) - goto error; } } + + if (!reservedVGA + && qemuDomainPCIAddressReserveSlot(addrs, 2) < 0) + goto error; + for (i = 0; i < def->nfss ; i++) { if (def->fss[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; -- 1.7.4

4 6

[libvirt] [PATCH] qemu: Support for overriding NPROC limit
by Jiri Denemark 06 Apr '11

06 Apr '11

This patch adds max_processes option to qemu.conf which can be used to override system default limit on number of processes that are allowed to be running for qemu user. --- src/qemu/libvirtd_qemu.aug | 3 +++ src/qemu/qemu.conf | 7 +++++++ src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_process.c | 24 ++++++++++++++++++++++++ src/qemu/test_libvirtd_qemu.aug | 4 ++++ 6 files changed, 44 insertions(+), 0 deletions(-) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index affd74e..ac30b8e 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -13,11 +13,13 @@ module Libvirtd_qemu = let str_val = del /\"/ "\"" . store /[^\"]*/ . del /\"/ "\"" let bool_val = store /0|1/ + let int_val = store /[0-9]+/ let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ "" let str_array_val = counter "el" . array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end let str_entry (kw:string) = [ key kw . value_sep . str_val ] let bool_entry (kw:string) = [ key kw . value_sep . bool_val ] + let int_entry (kw:string) = [ key kw . value_sep . int_val ] let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ] @@ -45,6 +47,7 @@ module Libvirtd_qemu = | bool_entry "clear_emulator_capabilities" | bool_entry "allow_disk_format_probing" | bool_entry "set_process_name" + | int_entry "max_processes" (* Each enty in the config is one of the following three ... *) let entry = vnc_entry diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 364f555..c70050e 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -273,3 +273,10 @@ # its arguments) appear in process listings. # # set_process_name = 1 + + +# If max_processes is set to a positive integer, libvirt will use it to set +# maximum number of processes that can be run by qemu user. This can be used to +# override default value set by host OS. +# +# max_processes = 0 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 9ba60b1..bb5421b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -424,6 +424,10 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, CHECK_TYPE ("set_process_name", VIR_CONF_LONG); if (p) driver->setProcessName = p->l; + p = virConfGetValue(conf, "max_processes"); + CHECK_TYPE("max_processes", VIR_CONF_LONG); + if (p) driver->maxProcesses = p->l; + virConfFree (conf); return 0; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 7c6fde7..94918f6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -105,6 +105,8 @@ struct qemud_driver { unsigned int allowDiskFormatProbing : 1; unsigned int setProcessName : 1; + int maxProcesses; + virCapsPtr caps; /* An array of callbacks */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 48ecd5c..9ada24d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -25,6 +25,8 @@ #include <unistd.h> #include <signal.h> #include <sys/stat.h> +#include <sys/time.h> +#include <sys/resource.h> #include "qemu_process.h" #include "qemu_domain.h" @@ -1811,6 +1813,25 @@ qemuProcessPrepareChardevDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, } +static int +qemuProcessLimits(struct qemud_driver *driver) +{ + if (driver->maxProcesses > 0) { + struct rlimit rlim; + + rlim.rlim_cur = rlim.rlim_max = driver->maxProcesses; + if (setrlimit(RLIMIT_NPROC, &rlim) < 0) { + virReportSystemError(errno, + _("cannot limit number of processes to %d"), + driver->maxProcesses); + return -1; + } + } + + return 0; +} + + struct qemuProcessHookData { virConnectPtr conn; virDomainObjPtr vm; @@ -1821,6 +1842,9 @@ static int qemuProcessHook(void *data) { struct qemuProcessHookData *h = data; + if (qemuProcessLimits(h->driver) < 0) + return -1; + /* This must take place before exec(), so that all QEMU * memory allocation is on the correct NUMA node */ diff --git a/src/qemu/test_libvirtd_qemu.aug b/src/qemu/test_libvirtd_qemu.aug index 8e477f5..917bd4f 100644 --- a/src/qemu/test_libvirtd_qemu.aug +++ b/src/qemu/test_libvirtd_qemu.aug @@ -111,6 +111,8 @@ clear_emulator_capabilities = 0 allow_disk_format_probing = 1 vnc_auto_unix_socket = 1 + +max_processes = 12345 " test Libvirtd_qemu.lns get conf = @@ -232,3 +234,5 @@ vnc_auto_unix_socket = 1 { "allow_disk_format_probing" = "1" } { "#empty" } { "vnc_auto_unix_socket" = "1" } +{ "#empty" } +{ "max_processes" = "12345" } -- 1.7.4.1

2 4

[libvirt] [libvirt-php] Fixed minor memory leak
by Lyre 06 Apr '11

06 Apr '11

--- src/libvirt-php.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libvirt-php.c b/src/libvirt-php.c index b6a848f..7870fcf 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -222,8 +222,8 @@ PHP_MINFO_FUNCTION(libvirt) if (virGetVersion(&libVer,NULL,NULL)== 0) { - version=emalloc(100); - snprintf(version, 100, "%i.%i.%i", (long)((libVer/1000000) % 1000),(long)((libVer/1000) % 1000),(long)(libVer % 1000)); + char version[100]; + snprintf(version, sizeof(version), "%i.%i.%i", (long)((libVer/1000000) % 1000),(long)((libVer/1000) % 1000),(long)(libVer % 1000)); php_info_print_table_row(2, "Libvirt version", version); } @@ -3883,10 +3883,12 @@ PHP_FUNCTION(libvirt_list_active_domains) if (name==NULL) { efree (ids); + virDomainFree (domain); RETURN_FALSE; } add_next_index_string(return_value, name, 1); + virDomainFree (domain); } } efree(ids); -- 1.7.3.4 v

2 1