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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list