[libvirt] [PATCH v3 1/2] qemu: Resolve data loss and data corruption of domain restoring.
by Osier Yang
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
13 years, 8 months
[libvirt] [PATCH v4] qemu: Unlink the managed state only if restoring succeeded
by Osier Yang
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
13 years, 8 months
[libvirt] the storage API requires capacity even using backingStore
by UbuntuKids China
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
13 years, 8 months
[libvirt] [PATCH] qemu: Do not unlink managedsave image if restoring fails.
by Osier Yang
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
13 years, 8 months
[libvirt] [PATCH] build: avoid compiler warning on cygwin
by Eric Blake
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
13 years, 8 months
[libvirt] [PATCH] Fix build for older gcc
by Jim Fehlig
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
13 years, 8 months
[libvirt] [PATCH 1/1] Change locking for udev monitor and callbacks
by Serge Hallyn
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
13 years, 8 months
[libvirt] [PATCH] qemu: Always reserves slot 0x02 for primary VGA.
by Osier Yang
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
13 years, 8 months
[libvirt] [PATCH] qemu: Support for overriding NPROC limit
by Jiri Denemark
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
13 years, 8 months
[libvirt] [libvirt-php] Fixed minor memory leak
by Lyre
---
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
13 years, 8 months
