1:12 a.m.
This patch series adds a new watchdog action `dump' which lets libvirtd
can do auto-dump when receiving a watchdog event from qemu guest.
In order to make the function work, there must be a watchdog device
added to guest, and guest must have a watchdog daemon running, for
example, /etc/init.d/watchdog start or auto-started on boot.
Changes from v2:
- let default auto-dump dir be /var/lib/libvirt/qemu/dump
Hu Tao (5):
Add a threadpool implementation
Fall back to QEMUD_SAVE_FORMAT_RAW if compression method fails.
Add a new function doCoreDump
Add a watchdog action `dump'
Add me to AUTHORS to make `make syntax-check' happy
AUTHORS | 1 +
src/Makefile.am | 3 +-
src/conf/domain_conf.c | 1 +
src/conf/domain_conf.h | 1 +
src/qemu/qemu.conf | 5 +
src/qemu/qemu_conf.c | 13 +++-
src/qemu/qemu_conf.h | 4 +
src/qemu/qemu_driver.c | 235 ++++++++++++++++++++++++++++++++++--------------
src/util/threadpool.c | 140 ++++++++++++++++++++++++++++
src/util/threadpool.h | 35 +++++++
10 files changed, 367 insertions(+), 71 deletions(-)
create mode 100644 src/util/threadpool.c
create mode 100644 src/util/threadpool.h
--
1.7.3
--
Thanks,
Hu Tao
1:12 a.m.
New subject: [libvirt] [PATCH v3 2/5] Fall back to QEMUD_SAVE_FORMAT_RAW if compression method fails.
When dumping a domain, it's reasonable to save dump-file in raw format
if dump format is misconfigured or the corresponding compress program
is not available rather then fail dumping.
---
src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++--------------
1 files changed, 25 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f00d8a3..ad67e52 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -137,6 +137,8 @@ struct _qemuDomainObjPrivate {
int persistentAddrs;
};
+static int getCompressionType(struct qemud_driver *driver);
+
static int qemudShutdown(void);
static void qemuDriverLock(struct qemud_driver *driver)
@@ -6055,16 +6057,10 @@ cleanup:
return ret;
}
-static int qemudDomainCoreDump(virDomainPtr dom,
- const char *path,
- int flags ATTRIBUTE_UNUSED) {
- struct qemud_driver *driver = dom->conn->privateData;
- virDomainObjPtr vm;
- int resume = 0, paused = 0;
- int ret = -1, fd = -1;
- virDomainEventPtr event = NULL;
+static int getCompressionType(struct qemud_driver *driver)
+{
int compress;
- qemuDomainObjPrivatePtr priv;
+
/*
* We reuse "save" flag for "dump" here. Then, we can support the
same
* format in "save" and "dump".
@@ -6073,18 +6069,33 @@ static int qemudDomainCoreDump(virDomainPtr dom,
if (driver->dumpImageFormat) {
compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat);
if (compress < 0) {
- qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("Invalid dump image format specified in "
- "configuration file"));
- return -1;
+ qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("Invalid dump image format specified in "
+ "configuration file"));
+ return QEMUD_SAVE_FORMAT_RAW;
}
if (!qemudCompressProgramAvailable(compress)) {
qemuReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("Compression program for dump image
format "
"in configuration file isn't
available"));
- return -1;
+ return QEMUD_SAVE_FORMAT_RAW;
}
}
+ return compress;
+}
+
+static int qemudDomainCoreDump(virDomainPtr dom,
+ const char *path,
+ int flags ATTRIBUTE_UNUSED) {
+ struct qemud_driver *driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ int resume = 0, paused = 0;
+ int ret = -1, fd = -1;
+ virDomainEventPtr event = NULL;
+ int compress;
+ qemuDomainObjPrivatePtr priv;
+
+ compress = getCompressionType(driver);
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
--
1.7.3
--
Thanks,
Hu Tao
4:19 p.m.
New subject: [libvirt] [PATCH v3 2/5] Fall back to QEMUD_SAVE_FORMAT_RAW if compression method fails.
On 11/30/2010 12:12 AM, Hu Tao wrote:
When dumping a domain, it's reasonable to save dump-file in raw
format
if dump format is misconfigured or the corresponding compress program
is not available rather then fail dumping.
Makes sense to me.
---
src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++++--------------
1 files changed, 25 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f00d8a3..ad67e52 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -137,6 +137,8 @@ struct _qemuDomainObjPrivate {
int persistentAddrs;
};
+static int getCompressionType(struct qemud_driver *driver);
This would be better as enum qemud_save_formats rather than int. No
need for a forward declaration, since you implemented the function in
topological order.
+static int qemudDomainCoreDump(virDomainPtr dom,
+ const char *path,
+ int flags ATTRIBUTE_UNUSED) {
+ struct qemud_driver *driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ int resume = 0, paused = 0;
+ int ret = -1, fd = -1;
+ virDomainEventPtr event = NULL;
+ int compress;
Likewise.
ACK with those nits fixed. So I squashed in 5/5 to keep 'make
syntax-check' happy for the purposes of 'git bisect', made the above
changes, and pushed this patch. The rest of your series still needs a v4.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:13 a.m.
New subject: [libvirt] [PATCH v3 3/5] Add a new function doCoreDump
This patch prepares for the next patch.
---
src/qemu/qemu_driver.c | 145 ++++++++++++++++++++++++++----------------------
1 files changed, 78 insertions(+), 67 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ad67e52..80ce9f6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -138,6 +138,7 @@ struct _qemuDomainObjPrivate {
};
static int getCompressionType(struct qemud_driver *driver);
+static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path,
int compress);
static int qemudShutdown(void);
@@ -6057,6 +6058,81 @@ cleanup:
return ret;
}
+static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path,
int compress)
+{
+ int fd = -1;
+ int ret = -1;
+ qemuDomainObjPrivatePtr priv;
+
+ priv = vm->privateData;
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
+ }
+
+ /* Create an empty file with appropriate ownership. */
+ if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ _("failed to create '%s'"), path);
+ goto endjob;
+ }
+
+ if (VIR_CLOSE(fd) < 0) {
+ virReportSystemError(errno,
+ _("unable to save file %s"),
+ path);
+ goto endjob;
+ }
+
+ if (driver->securityDriver &&
+ driver->securityDriver->domainSetSavedStateLabel &&
+
driver->securityDriver->domainSetSavedStateLabel(driver->securityDriver,
+ vm, path) == -1)
+ goto endjob;
+
+ qemuDomainObjEnterMonitorWithDriver(driver, vm);
+ if (compress == QEMUD_SAVE_FORMAT_RAW) {
+ const char *args[] = {
+ "cat",
+ NULL,
+ };
+ ret = qemuMonitorMigrateToFile(priv->mon,
+ QEMU_MONITOR_MIGRATE_BACKGROUND,
+ args, path, 0);
+ } else {
+ const char *prog = qemudSaveCompressionTypeToString(compress);
+ const char *args[] = {
+ prog,
+ "-c",
+ NULL,
+ };
+ ret = qemuMonitorMigrateToFile(priv->mon,
+ QEMU_MONITOR_MIGRATE_BACKGROUND,
+ args, path, 0);
+ }
+ qemuDomainObjExitMonitorWithDriver(driver, vm);
+ if (ret < 0)
+ goto endjob;
+
+ ret = qemuDomainWaitForMigrationComplete(driver, vm);
+
+ if (ret < 0)
+ goto endjob;
+
+ if (driver->securityDriver &&
+ driver->securityDriver->domainRestoreSavedStateLabel &&
+
driver->securityDriver->domainRestoreSavedStateLabel(driver->securityDriver,
+ vm, path) == -1)
+ goto endjob;
+
+endjob:
+ if (ret != 0)
+ unlink(path);
+ return ret;
+}
+
static int getCompressionType(struct qemud_driver *driver)
{
int compress;
@@ -6090,13 +6166,10 @@ static int qemudDomainCoreDump(virDomainPtr dom,
struct qemud_driver *driver = dom->conn->privateData;
virDomainObjPtr vm;
int resume = 0, paused = 0;
- int ret = -1, fd = -1;
+ int ret = -1;
virDomainEventPtr event = NULL;
- int compress;
qemuDomainObjPrivatePtr priv;
- compress = getCompressionType(driver);
-
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
@@ -6109,35 +6182,6 @@ static int qemudDomainCoreDump(virDomainPtr dom,
}
priv = vm->privateData;
- if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
- goto cleanup;
-
- if (!virDomainObjIsActive(vm)) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
- goto endjob;
- }
-
- /* Create an empty file with appropriate ownership. */
- if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) {
- qemuReportError(VIR_ERR_OPERATION_FAILED,
- _("failed to create '%s'"), path);
- goto endjob;
- }
-
- if (VIR_CLOSE(fd) < 0) {
- virReportSystemError(errno,
- _("unable to save file %s"),
- path);
- goto endjob;
- }
-
- if (driver->securityDriver &&
- driver->securityDriver->domainSetSavedStateLabel &&
-
driver->securityDriver->domainSetSavedStateLabel(driver->securityDriver,
- vm, path) == -1)
- goto endjob;
-
/* Migrate will always stop the VM, so the resume condition is
independent of whether the stop command is issued. */
resume = (vm->state == VIR_DOMAIN_RUNNING);
@@ -6161,43 +6205,12 @@ static int qemudDomainCoreDump(virDomainPtr dom,
}
}
- qemuDomainObjEnterMonitorWithDriver(driver, vm);
- if (compress == QEMUD_SAVE_FORMAT_RAW) {
- const char *args[] = {
- "cat",
- NULL,
- };
- ret = qemuMonitorMigrateToFile(priv->mon,
- QEMU_MONITOR_MIGRATE_BACKGROUND,
- args, path, 0);
- } else {
- const char *prog = qemudSaveCompressionTypeToString(compress);
- const char *args[] = {
- prog,
- "-c",
- NULL,
- };
- ret = qemuMonitorMigrateToFile(priv->mon,
- QEMU_MONITOR_MIGRATE_BACKGROUND,
- args, path, 0);
- }
- qemuDomainObjExitMonitorWithDriver(driver, vm);
- if (ret < 0)
- goto endjob;
-
- ret = qemuDomainWaitForMigrationComplete(driver, vm);
-
+ ret = doCoreDump(driver, vm, path, getCompressionType(driver));
if (ret < 0)
goto endjob;
paused = 1;
- if (driver->securityDriver &&
- driver->securityDriver->domainRestoreSavedStateLabel &&
-
driver->securityDriver->domainRestoreSavedStateLabel(driver->securityDriver,
- vm, path) == -1)
- goto endjob;
-
endjob:
if ((ret == 0) && (flags & VIR_DUMP_CRASH)) {
qemudShutdownVMDaemon(driver, vm, 0);
@@ -6230,8 +6243,6 @@ endjob:
}
cleanup:
- if (ret != 0)
- unlink(path);
if (vm)
virDomainObjUnlock(vm);
if (event)
--
1.7.3
--
Thanks,
Hu Tao
4:20 p.m.
New subject: [libvirt] [PATCH v3 3/5] Add a new function doCoreDump
On 11/30/2010 12:13 AM, Hu Tao wrote:
This patch prepares for the next patch.
---
src/qemu/qemu_driver.c | 145 ++++++++++++++++++++++++++----------------------
1 files changed, 78 insertions(+), 67 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ad67e52..80ce9f6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -138,6 +138,7 @@ struct _qemuDomainObjPrivate {
};
static int getCompressionType(struct qemud_driver *driver);
+static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path,
int compress);
static int qemudShutdown(void);
@@ -6057,6 +6058,81 @@ cleanup:
return ret;
}
+static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path,
int compress)
+{
+ int fd = -1;
+ int ret = -1;
+ qemuDomainObjPrivatePtr priv;
+
+ priv = vm->privateData;
You failed to move the qemuDomainObjBeginJobWithDriver() check to this
factored function, but deleted it from the caller - that's a surefire
way to crash the program, especially since other code remaining in in
qemudDomainCoreDump also wants to use qemuDomainObjEnterMonitorWithDriver.
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
+ }
Therefore, it's easier to leave this check in the caller, and assume
that doCoreDump will only be called by someone that already owns the
locks and has verified that vm is still running (but it does mean that
your new caller in 4/5 will have to comply with those assumptions).
+
+ /* Create an empty file with appropriate ownership. */
+ if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ _("failed to create '%s'"), path);
+ goto endjob;
+ }
At which point 'endjob' is not the best name for the label within
doCoreDump; sticking with 'cleanup' seems better.
@@ -6109,35 +6182,6 @@ static int qemudDomainCoreDump(virDomainPtr
dom,
}
priv = vm->privateData;
- if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
- goto cleanup;
-
- if (!virDomainObjIsActive(vm)) {
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
- goto endjob;
- }
In other words, this portion should probably not be moved out.
I do like the idea of this factorization though, so I'm looking forward
to v4.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:13 a.m.
New subject: [libvirt] [PATCH v3 4/5] Add a watchdog action `dump'
`dump' watchdog action lets libvirtd to dump the guest when receives a
watchdog event (which probably means a guest crash)
Currently only qemu is supported.
---
src/Makefile.am | 2 +-
src/conf/domain_conf.c | 1 +
src/conf/domain_conf.h | 1 +
src/qemu/qemu.conf | 5 +++
src/qemu/qemu_conf.c | 13 +++++++-
src/qemu/qemu_conf.h | 4 ++
src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
7 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am
index 5febd76..9484c2d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1089,7 +1089,7 @@ libvirt_test_la_LIBADD = $(libvirt_la_LIBADD)
libvirt_test_la_LDFLAGS = $(test_LDFLAGS) $(AM_LDFLAGS)
libvirt_test_la_CFLAGS = $(AM_CFLAGS)
-libvirt_qemu_la_SOURCES = libvirt-qemu.c
+libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
libvirt_qemu_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_QEMU_SYMBOL_FILE) \
-version-info $(LIBVIRT_VERSION_INFO) \
$(CYGWIN_EXTRA_LDFLAGS) $(MINGW_EXTRA_LDFLAGS) \
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3f14cee..a6cb444 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -245,6 +245,7 @@ VIR_ENUM_IMPL(virDomainWatchdogAction,
VIR_DOMAIN_WATCHDOG_ACTION_LAST,
"shutdown",
"poweroff",
"pause",
+ "dump",
"none")
VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 899b19f..7f50b79 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -462,6 +462,7 @@ enum virDomainWatchdogAction {
VIR_DOMAIN_WATCHDOG_ACTION_SHUTDOWN,
VIR_DOMAIN_WATCHDOG_ACTION_POWEROFF,
VIR_DOMAIN_WATCHDOG_ACTION_PAUSE,
+ VIR_DOMAIN_WATCHDOG_ACTION_DUMP,
VIR_DOMAIN_WATCHDOG_ACTION_NONE,
VIR_DOMAIN_WATCHDOG_ACTION_LAST
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index f4f965e..fb35ebc 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -191,6 +191,11 @@
# save_image_format = "raw"
# dump_image_format = "raw"
+# When a domain is configured to be auto-dumped when libvirtd receives a
+# watchdog event from qemu guest, libvirtd will saves dump files in directory
+# specified by auto_dump_path. Default value is /var/lib/libvirt/qemu/dump
+#
+# auto_dump_path = "/var/lib/libvirt/qemu/dump"
# If provided by the host and a hugetlbfs mount point is configured,
# a guest may request huge page backing. When this mount point is
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 35caccc..accd0c4 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -386,6 +386,17 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
}
}
+ p = virConfGetValue (conf, "auto_dump_path");
+ CHECK_TYPE ("auto_dump_path", VIR_CONF_STRING);
+ if (p && p->str) {
+ VIR_FREE(driver->autoDumpPath);
+ if (!(driver->autoDumpPath = strdup(p->str))) {
+ virReportOOMError();
+ virConfFree(conf);
+ return -1;
+ }
+ }
+
p = virConfGetValue (conf, "hugetlbfs_mount");
CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING);
if (p && p->str) {
@@ -5369,7 +5380,7 @@ int qemudBuildCommandLine(virConnectPtr conn,
}
ADD_ARG(optstr);
- const char *action = virDomainWatchdogActionTypeToString(watchdog->action);
+ const char *action = virDomainWatchdogActionTypeToString(watchdog->action ==
VIR_DOMAIN_WATCHDOG_ACTION_DUMP ? VIR_DOMAIN_WATCHDOG_ACTION_PAUSE :
watchdog->action);
if (!action) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("invalid watchdog action"));
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 790ce98..72f961a 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -106,6 +106,8 @@ enum qemud_cmd_flags {
struct qemud_driver {
virMutex lock;
+ struct virWorkerPool *workerPool;
+
int privileged;
uid_t user;
@@ -173,6 +175,8 @@ struct qemud_driver {
char *saveImageFormat;
char *dumpImageFormat;
+ char *autoDumpPath;
+
pciDeviceList *activePciHostdevs;
virBitmapPtr reservedVNCPorts;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 80ce9f6..550c6da 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -55,6 +55,7 @@
#include "qemu_driver.h"
#include "qemu_conf.h"
#include "qemu_monitor.h"
+#include "qemu_monitor_json.h"
#include "qemu_bridge_filter.h"
#include "c-ctype.h"
#include "event.h"
@@ -85,6 +86,7 @@
#include "files.h"
#include "fdstream.h"
#include "configmake.h"
+#include "threadpool.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -137,8 +139,15 @@ struct _qemuDomainObjPrivate {
int persistentAddrs;
};
+struct watchdogEvent
+{
+ virDomainObjPtr vm;
+ int action;
+};
+
static int getCompressionType(struct qemud_driver *driver);
static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path,
int compress);
+static void processWatchdogEvent(void *data);
static int qemudShutdown(void);
@@ -1206,6 +1215,16 @@ qemuHandleDomainWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0)
VIR_WARN("Unable to save status on vm %s after IO error",
vm->def->name);
}
+
+ if (vm->def->watchdog->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) {
+ struct watchdogEvent *wdEvent;
+ if (VIR_ALLOC(wdEvent) == 0) {
+ wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
+ wdEvent->vm = vm;
+ virWorkerPoolSendJob(driver->workerPool, wdEvent);
+ }
+ }
+
virDomainObjUnlock(vm);
if (watchdogEvent || lifecycleEvent) {
@@ -1788,6 +1807,9 @@ qemudStartup(int privileged) {
if (virAsprintf(&qemu_driver->snapshotDir,
"%s/lib/libvirt/qemu/snapshot", LOCALSTATEDIR) == -1)
goto out_of_memory;
+ if (virAsprintf(&qemu_driver->autoDumpPath,
+ "%s/lib/libvirt/qemu/dump", LOCALSTATEDIR) == -1)
+ goto out_of_memory;
} else {
uid_t uid = geteuid();
char *userdir = virGetUserDirectory(uid);
@@ -1816,6 +1838,8 @@ qemudStartup(int privileged) {
goto out_of_memory;
if (virAsprintf(&qemu_driver->snapshotDir, "%s/qemu/snapshot",
base) == -1)
goto out_of_memory;
+ if (virAsprintf(&qemu_driver->autoDumpPath, "%s/qemu/dump",
base) == -1)
+ goto out_of_memory;
}
if (virFileMakePath(qemu_driver->stateDir) != 0) {
@@ -1848,6 +1872,12 @@ qemudStartup(int privileged) {
qemu_driver->snapshotDir, virStrerror(errno, ebuf, sizeof ebuf));
goto error;
}
+ if (virFileMakePath(qemu_driver->autoDumpPath) != 0) {
+ char ebuf[1024];
+ VIR_ERROR(_("Failed to create dump dir '%s': %s"),
+ qemu_driver->autoDumpPath, virStrerror(errno, ebuf, sizeof ebuf));
+ goto error;
+ }
/* Configuration paths are either ~/.libvirt/qemu/... (session) or
* /etc/libvirt/qemu/... (system).
@@ -1973,6 +2003,10 @@ qemudStartup(int privileged) {
qemudAutostartConfigs(qemu_driver);
+ qemu_driver->workerPool = virWorkerPoolNew(0, 1, processWatchdogEvent);
+ if (!qemu_driver->workerPool)
+ goto error;
+
if (conn)
virConnectClose(conn);
@@ -2108,6 +2142,8 @@ qemudShutdown(void) {
qemuDriverUnlock(qemu_driver);
virMutexDestroy(&qemu_driver->lock);
+ if (qemu_driver->workerPool)
+ virWorkerPoolFree(qemu_driver->workerPool);
VIR_FREE(qemu_driver);
return 0;
@@ -4433,6 +4469,45 @@ retry:
}
}
+static void processWatchdogEvent(void *data)
+{
+ struct watchdogEvent *wdEvent = data;
+
+ switch (wdEvent->action) {
+ case VIR_DOMAIN_WATCHDOG_ACTION_DUMP:
+ {
+ char *dumpfile;
+ int i;
+
+ qemuDomainObjPrivatePtr priv = wdEvent->vm->privateData;
+
+ i = virAsprintf(&dumpfile, "%s/%s-%u",
qemu_driver->autoDumpPath, wdEvent->vm->def->name, (unsigned int)time(NULL));
+ dumpfile[i] = '\0';
+
+ qemuDriverLock(qemu_driver);
+ virDomainObjLock(wdEvent->vm);
+
+ if (qemuDomainObjBeginJobWithDriver(qemu_driver, wdEvent->vm) < 0)
+ break;
+
+ doCoreDump(qemu_driver, wdEvent->vm, dumpfile,
getCompressionType(qemu_driver));
+ qemuDomainObjEnterMonitorWithDriver(qemu_driver, wdEvent->vm);
+ qemuMonitorJSONStartCPUs(priv->mon, NULL);
+ qemuDomainObjExitMonitorWithDriver(qemu_driver, wdEvent->vm);
+
+ qemuDomainObjEndJob(wdEvent->vm);
+
+ virDomainObjUnlock(wdEvent->vm);
+ qemuDriverUnlock(qemu_driver);
+
+ VIR_FREE(dumpfile);
+ }
+ break;
+ }
+
+ VIR_FREE(wdEvent);
+}
+
static virDrvOpenStatus qemudOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags ATTRIBUTE_UNUSED) {
--
1.7.3
--
Thanks,
Hu Tao
4:21 p.m.
New subject: [libvirt] [PATCH v3 4/5] Add a watchdog action `dump'
On 11/30/2010 12:13 AM, Hu Tao wrote:
`dump' watchdog action lets libvirtd to dump the guest when
receives a
watchdog event (which probably means a guest crash)
Currently only qemu is supported.
---
src/Makefile.am | 2 +-
src/conf/domain_conf.c | 1 +
src/conf/domain_conf.h | 1 +
src/qemu/qemu.conf | 5 +++
src/qemu/qemu_conf.c | 13 +++++++-
src/qemu/qemu_conf.h | 4 ++
src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
7 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am
index 5febd76..9484c2d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1089,7 +1089,7 @@ libvirt_test_la_LIBADD = $(libvirt_la_LIBADD)
libvirt_test_la_LDFLAGS = $(test_LDFLAGS) $(AM_LDFLAGS)
libvirt_test_la_CFLAGS = $(AM_CFLAGS)
-libvirt_qemu_la_SOURCES = libvirt-qemu.c
+libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
Why is this change necessary? Shouldn't libvirt_util.la already include
threadpool.c, and the qemu driver already be linking with libvirt_util.la?
libvirt_qemu_la_LDFLAGS =
$(VERSION_SCRIPT_FLAGS)$(LIBVIRT_QEMU_SYMBOL_FILE) \
-version-info $(LIBVIRT_VERSION_INFO) \
$(CYGWIN_EXTRA_LDFLAGS) $(MINGW_EXTRA_LDFLAGS) \
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3f14cee..a6cb444 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -245,6 +245,7 @@ VIR_ENUM_IMPL(virDomainWatchdogAction,
VIR_DOMAIN_WATCHDOG_ACTION_LAST,
"shutdown",
"poweroff",
"pause",
+ "dump",
"none")
VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 899b19f..7f50b79 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -462,6 +462,7 @@ enum virDomainWatchdogAction {
VIR_DOMAIN_WATCHDOG_ACTION_SHUTDOWN,
VIR_DOMAIN_WATCHDOG_ACTION_POWEROFF,
VIR_DOMAIN_WATCHDOG_ACTION_PAUSE,
+ VIR_DOMAIN_WATCHDOG_ACTION_DUMP,
VIR_DOMAIN_WATCHDOG_ACTION_NONE,
VIR_DOMAIN_WATCHDOG_ACTION_LAST
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index f4f965e..fb35ebc 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -191,6 +191,11 @@
# save_image_format = "raw"
# dump_image_format = "raw"
+# When a domain is configured to be auto-dumped when libvirtd receives a
+# watchdog event from qemu guest, libvirtd will saves dump files in directory
s/saves/save/
+# specified by auto_dump_path. Default value is
/var/lib/libvirt/qemu/dump
+#
+# auto_dump_path = "/var/lib/libvirt/qemu/dump"
# If provided by the host and a hugetlbfs mount point is configured,
# a guest may request huge page backing. When this mount point is
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 35caccc..accd0c4 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -386,6 +386,17 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
}
}
+ p = virConfGetValue (conf, "auto_dump_path");
+ CHECK_TYPE ("auto_dump_path", VIR_CONF_STRING);
+ if (p && p->str) {
Where did you provide the default autoDumpPath if auto_dump_path is not
specified in the .conf file?
+ VIR_FREE(driver->autoDumpPath);
+ if (!(driver->autoDumpPath = strdup(p->str))) {
+ virReportOOMError();
+ virConfFree(conf);
+ return -1;
+ }
+ }
+
p = virConfGetValue (conf, "hugetlbfs_mount");
CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING);
if (p && p->str) {
@@ -5369,7 +5380,7 @@ int qemudBuildCommandLine(virConnectPtr conn,
}
ADD_ARG(optstr);
- const char *action = virDomainWatchdogActionTypeToString(watchdog->action);
+ const char *action = virDomainWatchdogActionTypeToString(watchdog->action ==
VIR_DOMAIN_WATCHDOG_ACTION_DUMP ? VIR_DOMAIN_WATCHDOG_ACTION_PAUSE : watchdog->action);
Please wrap to 80 columns when possible. For example, I would have done
something like:
{
int act = watchdog->action;
if (act == VIR_DOMAIN_WATCHDOG_ACTION_DUMP)
act = VIR_DOMAIN_WATCHDOG_ACTION_PAUSE;
const char *action = virDomainWatchdogActionTypeToString(act);
}
if (!action) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("invalid watchdog action"));
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 790ce98..72f961a 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -106,6 +106,8 @@ enum qemud_cmd_flags {
struct qemud_driver {
virMutex lock;
+ struct virWorkerPool *workerPool;
+
Where's the header that declares virWorkerPool?
int privileged;
uid_t user;
@@ -173,6 +175,8 @@ struct qemud_driver {
char *saveImageFormat;
char *dumpImageFormat;
+ char *autoDumpPath;
+
Memory leak if you don't add VIR_FREE(qemu_driver->autoDumpPath) to
qemudShutdown().
pciDeviceList *activePciHostdevs;
virBitmapPtr reservedVNCPorts;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 80ce9f6..550c6da 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -55,6 +55,7 @@
#include "qemu_driver.h"
#include "qemu_conf.h"
#include "qemu_monitor.h"
+#include "qemu_monitor_json.h"
Nope. The whole point of qemu_monitor.h is that it should isolate text
vs. json so that the rest of qemu_driver doesn't need to care about the
difference. That means you need to refactor things so that you provide
a qemu_monitor.c shim that either calls the json variant or gracefully
fails for the text variant.
#include "qemu_bridge_filter.h"
#include "c-ctype.h"
#include "event.h"
@@ -85,6 +86,7 @@
#include "files.h"
#include "fdstream.h"
#include "configmake.h"
+#include "threadpool.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -137,8 +139,15 @@ struct _qemuDomainObjPrivate {
int persistentAddrs;
};
+struct watchdogEvent
+{
+ virDomainObjPtr vm;
+ int action;
+};
+
static int getCompressionType(struct qemud_driver *driver);
static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path,
int compress);
+static void processWatchdogEvent(void *data);
static int qemudShutdown(void);
@@ -1206,6 +1215,16 @@ qemuHandleDomainWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0)
VIR_WARN("Unable to save status on vm %s after IO error",
vm->def->name);
}
+
+ if (vm->def->watchdog->action == VIR_DOMAIN_WATCHDOG_ACTION_DUMP) {
+ struct watchdogEvent *wdEvent;
+ if (VIR_ALLOC(wdEvent) == 0) {
+ wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
+ wdEvent->vm = vm;
+ virWorkerPoolSendJob(driver->workerPool, wdEvent);
+ }
+ }
You also need to handle allocation failure, by reporting the
out-of-memory condition rather than silently ignoring the watchdog.
+
virDomainObjUnlock(vm);
if (watchdogEvent || lifecycleEvent) {
@@ -1788,6 +1807,9 @@ qemudStartup(int privileged) {
if (virAsprintf(&qemu_driver->snapshotDir,
"%s/lib/libvirt/qemu/snapshot", LOCALSTATEDIR) == -1)
goto out_of_memory;
+ if (virAsprintf(&qemu_driver->autoDumpPath,
+ "%s/lib/libvirt/qemu/dump", LOCALSTATEDIR) == -1)
At first glance, I'm not quite sure why autoDumpPath is configurable but
not snapshotDir. I guess it has to do with the fact that snapshots are
under libvirt control (the user does not need to know that they exist),
but dump files are intended to be consumed by the user (so the user
should be able to specify an alternate location).
+ goto out_of_memory;
} else {
uid_t uid = geteuid();
char *userdir = virGetUserDirectory(uid);
@@ -1816,6 +1838,8 @@ qemudStartup(int privileged) {
goto out_of_memory;
if (virAsprintf(&qemu_driver->snapshotDir, "%s/qemu/snapshot",
base) == -1)
goto out_of_memory;
+ if (virAsprintf(&qemu_driver->autoDumpPath, "%s/qemu/dump",
base) == -1)
+ goto out_of_memory;
However, it does raise an issue. Is qemu.conf only for privileged
users, or do we have to worry about allowing non-privileged users also
be able to pick up an alternate directory (especially since they can't
dump to /var/log/...)?
}
if (virFileMakePath(qemu_driver->stateDir) != 0) {
@@ -1848,6 +1872,12 @@ qemudStartup(int privileged) {
qemu_driver->snapshotDir, virStrerror(errno, ebuf, sizeof ebuf));
goto error;
}
+ if (virFileMakePath(qemu_driver->autoDumpPath) != 0) {
+ char ebuf[1024];
+ VIR_ERROR(_("Failed to create dump dir '%s': %s"),
+ qemu_driver->autoDumpPath, virStrerror(errno, ebuf, sizeof ebuf));
+ goto error;
+ }
/* Configuration paths are either ~/.libvirt/qemu/... (session) or
* /etc/libvirt/qemu/... (system).
@@ -1973,6 +2003,10 @@ qemudStartup(int privileged) {
qemudAutostartConfigs(qemu_driver);
+ qemu_driver->workerPool = virWorkerPoolNew(0, 1, processWatchdogEvent);
+ if (!qemu_driver->workerPool)
+ goto error;
+
if (conn)
virConnectClose(conn);
@@ -2108,6 +2142,8 @@ qemudShutdown(void) {
qemuDriverUnlock(qemu_driver);
virMutexDestroy(&qemu_driver->lock);
+ if (qemu_driver->workerPool)
+ virWorkerPoolFree(qemu_driver->workerPool);
virWorkerPoolFree() should behave like free() and be a no-op if passed
NULL, in which case this if was not necessary. There's a list of
free()-like functions in cfg.mk, and virWorkerPoolFree should be added
to it.
VIR_FREE(qemu_driver);
return 0;
@@ -4433,6 +4469,45 @@ retry:
}
}
+static void processWatchdogEvent(void *data)
+{
+ struct watchdogEvent *wdEvent = data;
+
+ switch (wdEvent->action) {
+ case VIR_DOMAIN_WATCHDOG_ACTION_DUMP:
+ {
+ char *dumpfile;
+ int i;
+
+ qemuDomainObjPrivatePtr priv = wdEvent->vm->privateData;
+
+ i = virAsprintf(&dumpfile, "%s/%s-%u",
qemu_driver->autoDumpPath, wdEvent->vm->def->name, (unsigned int)time(NULL));
Wrap to 80 columns.
+ dumpfile[i] = '\0';
Not necessary; virAsprintf guarantees a NUL-terminated string.
+
+ qemuDriverLock(qemu_driver);
+ virDomainObjLock(wdEvent->vm);
+
+ if (qemuDomainObjBeginJobWithDriver(qemu_driver, wdEvent->vm) < 0)
+ break;
You need to check that vm is still active here, if your changes for
patch 3/5 move the active vm check out of doCoreDump.
+
+ doCoreDump(qemu_driver, wdEvent->vm, dumpfile,
getCompressionType(qemu_driver));
You should log any failures to complete the dump.
+ qemuDomainObjEnterMonitorWithDriver(qemu_driver,
wdEvent->vm);
+ qemuMonitorJSONStartCPUs(priv->mon, NULL);
This should call the shim in qemu_monitor.c, rather than directly
calling into qemuMonitorJSON*.
+ qemuDomainObjExitMonitorWithDriver(qemu_driver,
wdEvent->vm);
+
+ qemuDomainObjEndJob(wdEvent->vm);
+
+ virDomainObjUnlock(wdEvent->vm);
+ qemuDriverUnlock(qemu_driver);
+
+ VIR_FREE(dumpfile);
+ }
+ break;
+ }
+
+ VIR_FREE(wdEvent);
+}
+
static virDrvOpenStatus qemudOpen(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
int flags ATTRIBUTE_UNUSED) {
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:50 p.m.
New subject: [libvirt] [PATCH v3 4/5] Add a watchdog action `dump'
On Tue, Nov 30, 2010 at 03:21:36PM -0700, Eric Blake wrote:
<...snip...>
>
> -libvirt_qemu_la_SOURCES = libvirt-qemu.c
> +libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
Why is this change necessary? Shouldn't libvirt_util.la already include
threadpool.c, and the qemu driver already be linking with libvirt_util.la?
Is this ok?
-libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS)
+libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) ../src/libvirt_util.la
Or link will fail:
CCLD libvirtd
libvirtd-libvirtd.o: In function `qemudRunLoop':
/mnt/data/kernel/libvirt/daemon/libvirtd.c:2229: undefined reference to
`virWorkerPoolNew'
/mnt/data/kernel/libvirt/daemon/libvirtd.c:2287: undefined reference to
`virWorkerPoolFree'
libvirtd-libvirtd.o: In function `qemudDispatchClientRead':
/mnt/data/kernel/libvirt/daemon/libvirtd.c:1778: undefined reference to
`virWorkerPoolSendJob'
../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_driver.o): In function
`qemuHandleDomainWatchdog':
/mnt/data/kernel/libvirt/src/qemu/qemu_driver.c:1224: undefined reference to
`virWorkerPoolSendJob'
../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_driver.o): In function
`qemudShutdown':
/mnt/data/kernel/libvirt/src/qemu/qemu_driver.c:2147: undefined reference to
`virWorkerPoolFree'
../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_driver.o): In function
`qemudStartup':
/mnt/data/kernel/libvirt/src/qemu/qemu_driver.c:2007: undefined reference to
`virWorkerPoolNew'
collect2: ld returned 1 exit status
<...snip...>
> +
> virDomainObjUnlock(vm);
>
> if (watchdogEvent || lifecycleEvent) {
> @@ -1788,6 +1807,9 @@ qemudStartup(int privileged) {
> if (virAsprintf(&qemu_driver->snapshotDir,
> "%s/lib/libvirt/qemu/snapshot", LOCALSTATEDIR) ==
-1)
> goto out_of_memory;
> + if (virAsprintf(&qemu_driver->autoDumpPath,
> + "%s/lib/libvirt/qemu/dump", LOCALSTATEDIR) ==
-1)
At first glance, I'm not quite sure why autoDumpPath is configurable but
not snapshotDir. I guess it has to do with the fact that snapshots are
under libvirt control (the user does not need to know that they exist),
but dump files are intended to be consumed by the user (so the user
should be able to specify an alternate location).
Yes.
> + goto out_of_memory;
> } else {
> uid_t uid = geteuid();
> char *userdir = virGetUserDirectory(uid);
> @@ -1816,6 +1838,8 @@ qemudStartup(int privileged) {
> goto out_of_memory;
> if (virAsprintf(&qemu_driver->snapshotDir,
"%s/qemu/snapshot", base) == -1)
> goto out_of_memory;
> + if (virAsprintf(&qemu_driver->autoDumpPath,
"%s/qemu/dump", base) == -1)
> + goto out_of_memory;
However, it does raise an issue. Is qemu.conf only for privileged
users, or do we have to worry about allowing non-privileged users also
be able to pick up an alternate directory (especially since they can't
dump to /var/log/...)?
qemu.conf is only for privileged users, but non-privileged users who
need to analyze dump files should ask admin to specify an auto-dump
directory they have right to read.
Or do you have a better idea?
--
Thanks,
Hu Tao
7:39 p.m.
New subject: [libvirt] [PATCH v3 4/5] Add a watchdog action `dump'
On 12/01/2010 05:50 PM, Hu Tao wrote:
On Tue, Nov 30, 2010 at 03:21:36PM -0700, Eric Blake wrote:
<...snip...>
>>
>> -libvirt_qemu_la_SOURCES = libvirt-qemu.c
>> +libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
>
> Why is this change necessary? Shouldn't libvirt_util.la already include
> threadpool.c, and the qemu driver already be linking with libvirt_util.la?
Is this ok?
-libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS)
+libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) ../src/libvirt_util.la
Nope; rather...
Or link will fail:
CCLD libvirtd
libvirtd-libvirtd.o: In function `qemudRunLoop':
/mnt/data/kernel/libvirt/daemon/libvirtd.c:2229: undefined reference to
`virWorkerPoolNew'
/mnt/data/kernel/libvirt/daemon/libvirtd.c:2287: undefined reference to
`virWorkerPoolFree'
That means you need to modify src/libvirt_private.syms to export the new
public interfaces from threadpool.h (it should be pretty easy to figure
out what edits to make, the tricky part is realizing you need to touch
that file in the first place).
>> + if (virAsprintf(&qemu_driver->autoDumpPath,
"%s/qemu/dump", base) == -1)
>> + goto out_of_memory;
>
> However, it does raise an issue. Is qemu.conf only for privileged
> users, or do we have to worry about allowing non-privileged users also
> be able to pick up an alternate directory (especially since they can't
> dump to /var/log/...)?
qemu.conf is only for privileged users, but non-privileged users who
need to analyze dump files should ask admin to specify an auto-dump
directory they have right to read.
Or do you have a better idea?
Is it better to dump a non-privileged log into ~/.libvirt/qemu/dump, so
that it's automatically user-accessible? We already use ~/.libvirt for
other non-privileged files.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:48 a.m.
New subject: [libvirt] [PATCH v3 4/5] Add a watchdog action `dump'
On Wed, Dec 01, 2010 at 06:39:14PM -0700, Eric Blake wrote:
On 12/01/2010 05:50 PM, Hu Tao wrote:
> On Tue, Nov 30, 2010 at 03:21:36PM -0700, Eric Blake wrote:
> <...snip...>
>>>
>>> -libvirt_qemu_la_SOURCES = libvirt-qemu.c
>>> +libvirt_qemu_la_SOURCES = libvirt-qemu.c util/threadpool.c
>>
>> Why is this change necessary? Shouldn't libvirt_util.la already include
>> threadpool.c, and the qemu driver already be linking with libvirt_util.la?
>
> Is this ok?
>
> -libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS)
> +libvirt_driver_qemu_la_LIBADD = $(NUMACTL_LIBS) ../src/libvirt_util.la
Nope; rather...
>
> Or link will fail:
>
> CCLD libvirtd
> libvirtd-libvirtd.o: In function `qemudRunLoop':
> /mnt/data/kernel/libvirt/daemon/libvirtd.c:2229: undefined reference to
`virWorkerPoolNew'
> /mnt/data/kernel/libvirt/daemon/libvirtd.c:2287: undefined reference to
`virWorkerPoolFree'
That means you need to modify src/libvirt_private.syms to export the new
public interfaces from threadpool.h (it should be pretty easy to figure
out what edits to make, the tricky part is realizing you need to touch
that file in the first place).
>>> + if (virAsprintf(&qemu_driver->autoDumpPath,
"%s/qemu/dump", base) == -1)
>>> + goto out_of_memory;
>>
>> However, it does raise an issue. Is qemu.conf only for privileged
>> users, or do we have to worry about allowing non-privileged users also
>> be able to pick up an alternate directory (especially since they can't
>> dump to /var/log/...)?
>
> qemu.conf is only for privileged users, but non-privileged users who
> need to analyze dump files should ask admin to specify an auto-dump
> directory they have right to read.
>
> Or do you have a better idea?
Is it better to dump a non-privileged log into ~/.libvirt/qemu/dump, so
that it's automatically user-accessible? We already use ~/.libvirt for
other non-privileged files.
I think you're mixing up unprivileged users using the privileged
qemu:///system, with unprivileged users using the unprivileged
driver qemu://session. Only the latter uses ~/.libvirt and this
patch should already be using ~/.libvirt/qemu/dump in that
scenario.
Daniel
1:13 a.m.
New subject: [libvirt] [PATCH v3 5/5] Add me to AUTHORS to make `make syntax-check' happy
---
AUTHORS | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index 449a450..3534dcb 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -135,6 +135,7 @@ Patches have also been contributed by:
John Morrissey <jwm(a)horde.net>
Osier Yang <jyang(a)redhat.com>
Kamezawa Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com>
+ Hu Tao <hutao(a)cn.fujitsu.com>
[....send patches to get your name here....]
--
1.7.3
--
Thanks,
Hu Tao
1:14 a.m.
New subject: [libvirt] [PATCH v3 1/5] Add a threadpool implementation
---
src/Makefile.am | 1 +
src/util/threadpool.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
src/util/threadpool.h | 35 ++++++++++++
3 files changed, 176 insertions(+), 0 deletions(-)
create mode 100644 src/util/threadpool.c
create mode 100644 src/util/threadpool.h
diff --git a/src/Makefile.am b/src/Makefile.am
index a9a1986..5febd76 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -76,6 +76,7 @@ UTIL_SOURCES = \
util/uuid.c util/uuid.h \
util/util.c util/util.h \
util/xml.c util/xml.h \
+ util/threadpool.c util/threadpool.h \
util/virtaudit.c util/virtaudit.h \
util/virterror.c util/virterror_internal.h
diff --git a/src/util/threadpool.c b/src/util/threadpool.c
new file mode 100644
index 0000000..4bf0f8d
--- /dev/null
+++ b/src/util/threadpool.c
@@ -0,0 +1,140 @@
+#include <config.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "threadpool.h"
+
+static void *workerHandleJob(void *data)
+{
+ struct virData *localData = NULL;
+ struct virWorkerPool *pool = data;
+
+ pthread_mutex_lock(&pool->mutex);
+
+ while (1) {
+ while (!pool->quit && !pool->dataList) {
+ pool->nFreeWorker++;
+ pthread_cond_signal(&pool->worker_cond);
+ pthread_cond_wait(&pool->cond, &pool->mutex);
+ pool->nFreeWorker--;
+
+ if (pool->nWorker > pool->nMaxWorker)
+ goto out;
+ }
+
+ while ((localData = pool->dataList) != NULL) {
+ pool->dataList = pool->dataList->next;
+ localData->next = NULL;
+
+ pthread_mutex_unlock(&pool->mutex);
+
+ (pool->func)(localData->data);
+ free(localData);
+
+ pthread_mutex_lock(&pool->mutex);
+ }
+
+ if (pool->quit)
+ break;
+ }
+
+out:
+ pool->nWorker--;
+ if (pool->nWorker == 0)
+ pthread_cond_signal(&pool->quit_cond);
+ pthread_mutex_unlock(&pool->mutex);
+
+ return NULL;
+}
+
+struct virWorkerPool *virWorkerPoolNew(int nWorker, int maxWorker, virWorkerFunc func)
+{
+ struct virWorkerPool *pool;
+ pthread_t pid;
+ int i;
+
+ if (nWorker < 0)
+ return NULL;
+
+ if (nWorker > maxWorker)
+ return NULL;
+
+ pool = malloc(sizeof(*pool));
+ if (!pool)
+ return NULL;
+
+ memset(pool, 0, sizeof(*pool));
+ pool->func = func;
+ pthread_mutex_init(&pool->mutex, NULL);
+ pthread_cond_init(&pool->cond, NULL);
+ pthread_cond_init(&pool->worker_cond, NULL);
+ pthread_cond_init(&pool->quit_cond, NULL);
+
+ for (i = 0; i < nWorker; i++) {
+ pthread_create(&pid, NULL, workerHandleJob, pool);
+ }
+
+ pool->nFreeWorker = 0;
+ pool->nWorker = nWorker;
+ pool->nMaxWorker = maxWorker;
+
+ return pool;
+}
+
+void virWorkerPoolFree(struct virWorkerPool *pool)
+{
+ pthread_mutex_lock(&pool->mutex);
+ pool->quit = 1;
+ if (pool->nWorker > 0) {
+ pthread_cond_broadcast(&pool->cond);
+ pthread_cond_wait(&pool->quit_cond, &pool->mutex);
+ }
+ pthread_mutex_unlock(&pool->mutex);
+ free(pool);
+}
+
+int virWorkerPoolSendJob(struct virWorkerPool *pool, void *data)
+{
+ pthread_t pid;
+ struct virData *localData;
+
+ localData = malloc(sizeof(*localData));
+ if (!localData)
+ return -1;
+
+ localData->data = data;
+
+ pthread_mutex_lock(&pool->mutex);
+ if (pool->quit) {
+ pthread_mutex_unlock(&pool->mutex);
+ free(localData);
+ return -1;
+ }
+
+ localData->next = pool->dataList;
+ pool->dataList = localData;
+
+ if (pool->nFreeWorker == 0 && pool->nWorker < pool->nMaxWorker)
{
+ pthread_create(&pid, NULL, workerHandleJob, pool);
+ pool->nWorker++;
+ }
+
+ pthread_cond_signal(&pool->cond);
+
+ pthread_mutex_unlock(&pool->mutex);
+
+ return 0;
+}
+
+int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker)
+{
+ if (maxWorker < 0)
+ return -1;
+
+ pthread_mutex_lock(&pool->mutex);
+ pool->nMaxWorker = maxWorker;
+ pthread_mutex_unlock(&pool->mutex);
+
+ return 0;
+}
diff --git a/src/util/threadpool.h b/src/util/threadpool.h
new file mode 100644
index 0000000..5ff3a6b
--- /dev/null
+++ b/src/util/threadpool.h
@@ -0,0 +1,35 @@
+#ifndef __THREADPOOL_H__
+#define __THREADPOOL_H__
+
+#include <pthread.h>
+
+typedef void (*virWorkerFunc)(void *);
+
+struct virData {
+ struct virData *next;
+
+ void *data;
+};
+
+struct virWorkerPool {
+ int nWorker;
+ int nMaxWorker;
+ int nFreeWorker;
+
+ int quit;
+
+ virWorkerFunc func;
+ struct virData *dataList;
+
+ pthread_mutex_t mutex;
+ pthread_cond_t cond;
+ pthread_cond_t worker_cond;
+ pthread_cond_t quit_cond;
+};
+
+struct virWorkerPool *virWorkerPoolNew(int nWorker, int nMaxWorker, virWorkerFunc func);
+void virWorkerPoolFree(struct virWorkerPool *pool);
+int virWorkerPoolSendJob(struct virWorkerPool *wp, void *data);
+int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker);
+
+#endif
--
1.7.3
--
Thanks,
Hu Tao
2:54 p.m.
New subject: [libvirt] [PATCH v3 1/5] Add a threadpool implementation
On 11/30/2010 12:14 AM, Hu Tao wrote:
---
src/Makefile.am | 1 +
src/util/threadpool.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
src/util/threadpool.h | 35 ++++++++++++
3 files changed, 176 insertions(+), 0 deletions(-)
create mode 100644 src/util/threadpool.c
create mode 100644 src/util/threadpool.h
diff --git a/src/Makefile.am b/src/Makefile.am
index a9a1986..5febd76 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -76,6 +76,7 @@ UTIL_SOURCES = \
util/uuid.c util/uuid.h \
util/util.c util/util.h \
util/xml.c util/xml.h \
+ util/threadpool.c util/threadpool.h \
util/virtaudit.c util/virtaudit.h \
util/virterror.c util/virterror_internal.h
diff --git a/src/util/threadpool.c b/src/util/threadpool.c
new file mode 100644
index 0000000..4bf0f8d
--- /dev/null
+++ b/src/util/threadpool.c
@@ -0,0 +1,140 @@
Copyright header?
+#include <config.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "threadpool.h"
+
+static void *workerHandleJob(void *data)
+{
+ struct virData *localData = NULL;
+ struct virWorkerPool *pool = data;
+
+ pthread_mutex_lock(&pool->mutex);
We should be using virMutexLock here, so as to also be portable to mingw.
+
+ while (1) {
+ while (!pool->quit && !pool->dataList) {
+ pool->nFreeWorker++;
+ pthread_cond_signal(&pool->worker_cond);
Likewise, virCondSignal here.
+ pthread_cond_wait(&pool->cond,
&pool->mutex);
and virCondWait.
+ pool->nFreeWorker--;
+
+ if (pool->nWorker > pool->nMaxWorker)
+ goto out;
+ }
+
+ while ((localData = pool->dataList) != NULL) {
+ pool->dataList = pool->dataList->next;
+ localData->next = NULL;
+
+ pthread_mutex_unlock(&pool->mutex);
+
+ (pool->func)(localData->data);
+ free(localData);
VIR_FREE().
+
+ pthread_mutex_lock(&pool->mutex);
+ }
+
+ if (pool->quit)
+ break;
+ }
+
+out:
+ pool->nWorker--;
+ if (pool->nWorker == 0)
+ pthread_cond_signal(&pool->quit_cond);
+ pthread_mutex_unlock(&pool->mutex);
+
+ return NULL;
+}
+
+struct virWorkerPool *virWorkerPoolNew(int nWorker, int maxWorker, virWorkerFunc func)
+{
+ struct virWorkerPool *pool;
+ pthread_t pid;
+ int i;
+
+ if (nWorker < 0)
+ return NULL;
+
+ if (nWorker > maxWorker)
+ return NULL;
daemon/libvirtd.c already has a notion of worker threads; I'm wondering
how much overlap there is between your implementation and that one. A
better proof that this would be a useful API addition would be to have
the next patch in the series convert libvirtd.c over to using this API.
+
+ pool = malloc(sizeof(*pool));
Run 'make syntax-check' - it would have complained about this. Use
VIR_ALLOC or VIR_ALLOC_N instead of malloc.
+ if (!pool)
+ return NULL;
+
+ memset(pool, 0, sizeof(*pool));
+ pool->func = func;
+ pthread_mutex_init(&pool->mutex, NULL);
virMutexInit()
+ pthread_cond_init(&pool->cond, NULL);
+ pthread_cond_init(&pool->worker_cond, NULL);
+ pthread_cond_init(&pool->quit_cond, NULL);
virCondInit()
+
+ for (i = 0; i < nWorker; i++) {
+ pthread_create(&pid, NULL, workerHandleJob, pool);
virThreadCreate()
+ }
+
+ pool->nFreeWorker = 0;
+ pool->nWorker = nWorker;
+ pool->nMaxWorker = maxWorker;
+
+ return pool;
+}
+
+void virWorkerPoolFree(struct virWorkerPool *pool)
+{
+ pthread_mutex_lock(&pool->mutex);
+ pool->quit = 1;
Use <stdbool.h> and bool if a value will only ever be 0 or 1.
+ if (pool->nWorker > 0) {
+ pthread_cond_broadcast(&pool->cond);
+ pthread_cond_wait(&pool->quit_cond, &pool->mutex);
+ }
+ pthread_mutex_unlock(&pool->mutex);
+ free(pool);
VIR_FREE()
+}
+
+int virWorkerPoolSendJob(struct virWorkerPool *pool, void *data)
+{
+ pthread_t pid;
+ struct virData *localData;
+
+ localData = malloc(sizeof(*localData));
VIR_ALLOC()
+ if (!localData)
+ return -1;
+
+ localData->data = data;
+
+ pthread_mutex_lock(&pool->mutex);
+ if (pool->quit) {
+ pthread_mutex_unlock(&pool->mutex);
+ free(localData);
+ return -1;
+ }
+
+ localData->next = pool->dataList;
+ pool->dataList = localData;
+
+ if (pool->nFreeWorker == 0 && pool->nWorker < pool->nMaxWorker)
{
+ pthread_create(&pid, NULL, workerHandleJob, pool);
+ pool->nWorker++;
+ }
+
+ pthread_cond_signal(&pool->cond);
+
+ pthread_mutex_unlock(&pool->mutex);
+
+ return 0;
+}
+
+int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker)
+{
+ if (maxWorker < 0)
+ return -1;
+
+ pthread_mutex_lock(&pool->mutex);
+ pool->nMaxWorker = maxWorker;
+ pthread_mutex_unlock(&pool->mutex);
Does this do the right thing if maxWorker < pool->nMaxWorker, or does it
silently lose existing workers?
+
+ return 0;
+}
diff --git a/src/util/threadpool.h b/src/util/threadpool.h
new file mode 100644
index 0000000..5ff3a6b
--- /dev/null
+++ b/src/util/threadpool.h
@@ -0,0 +1,35 @@
Copyright header?
+#ifndef __THREADPOOL_H__
+#define __THREADPOOL_H__
Use of the __ namespace risks collision with the system; I'd feel better
if this were __VIR_THREADPOOL_H__.
+
+#include <pthread.h>
"threads.h", not <pthread.h>, so we can support mingw
+
+typedef void (*virWorkerFunc)(void *);
pthread_create() takes a function that can return void*. Should worker
functions be allowed to return a value?
+
+struct virData {
+ struct virData *next;
+
+ void *data;
+};
We've typically used typedefs to avoid having to type 'struct virData'
everywhere else.
+
+struct virWorkerPool {
+ int nWorker;
+ int nMaxWorker;
+ int nFreeWorker;
s/int/size_t/ when dealing with non-zero counts.
+
+ int quit;
s/int/bool/
+
+ virWorkerFunc func;
+ struct virData *dataList;
+
+ pthread_mutex_t mutex;
+ pthread_cond_t cond;
+ pthread_cond_t worker_cond;
+ pthread_cond_t quit_cond;
virMutex, virCond
+};
+
+struct virWorkerPool *virWorkerPoolNew(int nWorker, int nMaxWorker, virWorkerFunc func);
needs ATTRIBUTE_RETURN_CHECK.
+void virWorkerPoolFree(struct virWorkerPool *pool);
+int virWorkerPoolSendJob(struct virWorkerPool *wp, void *data);
ATTRIBUTE_NONNULL(1)
+int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int
maxWorker);
ATTRIBUTE_NONNULL(1)
+
+#endif
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:32 a.m.
New subject: [libvirt] [PATCH v3 1/5] Add a threadpool implementation
Hi Eric,
Thanks for your careful review of these patches. I'll post v4 patches
tomorrow fixing all problems you pointed out.
daemon/libvirtd.c already has a notion of worker threads; I'm wondering
how much overlap there is between your implementation and that one. A
better proof that this would be a useful API addition would be to have
the next patch in the series convert libvirtd.c over to using this API.
OK. Will be in v4.
<...snip...>
> +int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int
maxWorker)
> +{
> + if (maxWorker < 0)
> + return -1;
> +
> + pthread_mutex_lock(&pool->mutex);
> + pool->nMaxWorker = maxWorker;
> + pthread_mutex_unlock(&pool->mutex);
Does this do the right thing if maxWorker < pool->nMaxWorker, or does it
silently lose existing workers?
In the case maxWorker < pool->nMaxWorker and there are pool->nMaxWorker
threads running, (pool->nMaxWorker - maxWorker) threads will exit after
the new nMaxWorker set.
<...snip...>
> +
> +typedef void (*virWorkerFunc)(void *);
pthread_create() takes a function that can return void*. Should worker
functions be allowed to return a value?
threadpool doesn't care the return value, neither it has no way to pass
the return value to threadpool creator, so it's meaningless for worker
functions to return a value.
Another example is virThreadFunc which does't return a value neither.
--
Thanks,
Hu Tao
4:03 a.m.
New subject: [libvirt] [PATCH v3 1/5] Add a threadpool implementation
On Wed, Dec 01, 2010 at 05:32:44PM +0800, Hu Tao wrote:
Hi Eric,
Thanks for your careful review of these patches. I'll post v4 patches
tomorrow fixing all problems you pointed out.
>
> daemon/libvirtd.c already has a notion of worker threads; I'm wondering
> how much overlap there is between your implementation and that one. A
> better proof that this would be a useful API addition would be to have
> the next patch in the series convert libvirtd.c over to using this API.
OK. Will be in v4.
<...snip...>
> > +int virWorkerPoolSetMaxWorker(struct virWorkerPool *pool, int maxWorker)
> > +{
> > + if (maxWorker < 0)
> > + return -1;
> > +
> > + pthread_mutex_lock(&pool->mutex);
> > + pool->nMaxWorker = maxWorker;
> > + pthread_mutex_unlock(&pool->mutex);
>
> Does this do the right thing if maxWorker < pool->nMaxWorker, or does it
> silently lose existing workers?
In the case maxWorker < pool->nMaxWorker and there are pool->nMaxWorker
threads running, (pool->nMaxWorker - maxWorker) threads will exit after
the new nMaxWorker set.
<...snip...>
> > +
> > +typedef void (*virWorkerFunc)(void *);
>
> pthread_create() takes a function that can return void*. Should worker
> functions be allowed to return a value?
threadpool doesn't care the return value, neither it has no way to pass
the return value to threadpool creator, so it's meaningless for worker
functions to return a value.
Another example is virThreadFunc which does't return a value neither.
I've needed a thread pool implementation for an unrelated piece
of work I'm doing on libvirt. I took your impl here, and updated
it to follow libvirt naming style, use appropriate internals APIs,
and hide the struct definitions from the header. Take a look at
the files attached.
Regards,
Daniel
5103
days inactive
5105
days old
14 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Hu Tao