Sometimes when guest machine crashes, coredump can get huge due to the guest memory. This can be limited using madvise(2) system call and is being used in QEMU hypervisor. This patch adds an option for configuring that in the domain XML and related documentation. --- docs/formatdomain.html.in | 12 +++++++++--- docs/schemas/domaincommon.rng | 8 ++++++++ src/conf/domain_conf.c | 25 ++++++++++++++++++++++++- src/conf/domain_conf.h | 10 ++++++++++ src/libvirt_private.syms | 2 ++ 5 files changed, 53 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d021837..210ebe0 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -515,9 +515,15 @@ However, the value will be rounded up to the nearest kibibyte by libvirt, and may be further rounded to the granularity supported by the hypervisor. Some hypervisors also enforce a - minimum, such as - 4000KiB. <span class='since'><code>unit</code> since - 0.9.11</span></dd> + minimum, such as 4000KiB. + + In the case of crash, optional attribute <code>dump-core</code> + can be used to control whether the guest memory should be + included in the generated coredump or not (values "on", "off"). + + <span class='since'><code>unit</code> since 0.9.11</span>, + <span class='since'><code>dump-core</code> since 0.10.2 + (QEMU only)</span></dd> <dt><code>currentMemory</code></dt> <dd>The actual allocation of memory for the guest. This value can be less than the maximum allocation, to allow for ballooning diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ed25f58..bf6afbb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -470,6 +470,14 @@ <interleave> <element name="memory"> <ref name='scaledInteger'/> + <optional> + <attribute name="dump-core"> + <choice> + <value>on</value> + <value>off</value> + </choice> + </attribute> + </optional> </element> <optional> <element name="currentMemory"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4714312..c0cff7b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -381,6 +381,11 @@ VIR_ENUM_IMPL(virDomainSoundModel, VIR_DOMAIN_SOUND_MODEL_LAST, "ac97", "ich6") +VIR_ENUM_IMPL(virDomainMemDump, VIR_DOMAIN_MEM_DUMP_LAST, + "default", + "on", + "off") + VIR_ENUM_IMPL(virDomainMemballoonModel, VIR_DOMAIN_MEMBALLOON_MODEL_LAST, "virtio", "xen", @@ -8525,6 +8530,19 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->mem.cur_balloon, false) < 0) goto error; + /* and info about it */ + tmp = virXPathString("string(./memory[1]/@dump-core)", ctxt); + if (tmp) { + def->mem.dump_core = virDomainMemDumpTypeFromString(tmp); + VIR_FREE(tmp); + + if (def->mem.dump_core < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid value for <memory> 'dump-core' attribute")); + goto error; + } + } + if (def->mem.cur_balloon > def->mem.max_balloon) { /* Older libvirt could get into this situation due to * rounding; if the discrepancy is less than 1MiB, we silently @@ -13267,8 +13285,13 @@ virDomainDefFormatInternal(virDomainDefPtr def, xmlIndentTreeOutput = oldIndentTreeOutput; } - virBufferAsprintf(buf, " <memory unit='KiB'>%llu</memory>\n", + virBufferAddLit(buf, " <memory"); + if (def->mem.dump_core) + virBufferAsprintf(buf, " dump-core='%s'", + virDomainMemDumpTypeToString(def->mem.dump_core)); + virBufferAsprintf(buf, " unit='KiB'>%llu</memory>\n", def->mem.max_balloon); + virBufferAsprintf(buf, " <currentMemory unit='KiB'>%llu</currentMemory>\n", def->mem.cur_balloon); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d719d57..fae699f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1320,6 +1320,14 @@ struct _virDomainRedirFilterDef { virDomainRedirFilterUsbDevDefPtr *usbdevs; }; +enum virDomainMemDump { + VIR_DOMAIN_MEM_DUMP_DEFAULT = 0, + VIR_DOMAIN_MEM_DUMP_ON, + VIR_DOMAIN_MEM_DUMP_OFF, + + VIR_DOMAIN_MEM_DUMP_LAST, +}; + enum { VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO, VIR_DOMAIN_MEMBALLOON_MODEL_XEN, @@ -1641,6 +1649,7 @@ struct _virDomainDef { unsigned long long max_balloon; /* in kibibytes */ unsigned long long cur_balloon; /* in kibibytes */ bool hugepage_backed; + int dump_core; /* enum virDomainMemDump */ unsigned long long hard_limit; /* in kibibytes */ unsigned long long soft_limit; /* in kibibytes */ unsigned long long min_guarantee; /* in kibibytes */ @@ -2177,6 +2186,7 @@ VIR_ENUM_DECL(virDomainChrTcpProtocol) VIR_ENUM_DECL(virDomainChrSpicevmc) VIR_ENUM_DECL(virDomainSoundCodec) VIR_ENUM_DECL(virDomainSoundModel) +VIR_ENUM_DECL(virDomainMemDump) VIR_ENUM_DECL(virDomainMemballoonModel) VIR_ENUM_DECL(virDomainSmbiosMode) VIR_ENUM_DECL(virDomainWatchdogModel) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0b53895..0b6068d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -426,6 +426,8 @@ virDomainLiveConfigHelperMethod; virDomainLoadAllConfigs; virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; +virDomainMemDumpTypeFromString; +virDomainMemDumpTypeToString; virDomainNetDefFree; virDomainNetFind; virDomainNetGetActualBandwidth; -- 1.7.12

On 09/20/2012 11:32 AM, Daniel P. Berrange wrote: I didn't realize that, OK. It's fixed, should I send v3 for review because of that? Martin

On 20.09.2012 10:58, Martin Kletzander wrote: s/-c/C/ as Dan pointed out s/</<=/ because we don't want to let users type in 'default' we tend to write the value passed by user: virReportError(VIR_ERR_XML_ERROR, _("Bad value '%s'"), tmp); but that requires you won't free 'tmp' just a few lines above. ACK if you fix those nits. Michal

The "dump-guest-core' option is new option for the machine type (-machine pc,dump-guest-core) that controls whether the guest memory will be marked as dumpable. While testing this, I've found out that the value for the '-M' options is not parsed correctly when additional parameters are used. However, when '-machine' is used for the same options, it gets parsed as expected. That's why this patch also modifies the parsing and creating of the command line, so both '-M' and '-machine' are recognized. In QEMU's help there is only mention of the 'machine parameter now with no sign of the older '-M'. --- src/qemu/qemu_capabilities.c | 4 +++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 81 +++++++++++++++++++++++++++++++++++++++----- tests/qemuhelptest.c | 3 +- 4 files changed, 79 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3582cbd..4b52dc5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -182,6 +182,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "seccomp-sandbox", "reboot-timeout", /* 110 */ + "dump-guest-core", ); struct _qemuCaps { @@ -1243,6 +1244,9 @@ qemuCapsComputeCmdFlags(const char *help, if (strstr(help, "-no-shutdown") && (version < 14000 || version > 15000)) qemuCapsSet(caps, QEMU_CAPS_NO_SHUTDOWN); + if (strstr(help, "dump-guest-core=on|off")) + qemuCapsSet(caps, QEMU_CAPS_DUMP_GUEST_CORE); + /* * Handling of -incoming arg with varying features * -incoming tcp (kvm >= 79, qemu >= 0.10.0) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 2201cb3..a069f42 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -146,6 +146,7 @@ enum qemuCapsFlags { QEMU_CAPS_SCSI_DISK_WWN = 108, /* Is scsi-disk.wwn available? */ QEMU_CAPS_SECCOMP_SANDBOX = 109, /* -sandbox */ QEMU_CAPS_REBOOT_TIMEOUT = 110, /* -boot reboot-timeout */ + QEMU_CAPS_DUMP_GUEST_CORE = 111, /* dump-guest-core-parameter */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3807596..52ad4d2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4306,6 +4306,45 @@ no_memory: goto cleanup; } +static int +qemuBuildMachineArgStr(virCommandPtr cmd, + const virDomainDefPtr def, + qemuCapsPtr caps) +{ + /* This should *never* be NULL, since we always provide + * a machine in the capabilities data for QEMU. So this + * check is just here as a safety in case the unexpected + * happens */ + if (!def->os.machine) + return 0; + + if (!def->mem.dump_core) { + /* if no parameter to the machine type is needed, we still use + * '-M' to keep the most of the compatibility with older versions. + */ + virCommandAddArgList(cmd, "-M", def->os.machine, NULL); + } else { + if (!qemuCapsGet(caps, QEMU_CAPS_DUMP_GUEST_CORE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("dump-guest-core is not available " + " with this QEMU binary")); + return -1; + } + + /* However, in case there is a parameter to be added, we need to + * use the "-machine" parameter because qemu is not parsing the + * "-M" correctly */ + virCommandAddArg(cmd, "-machine"); + virCommandAddArgFormat(cmd, + "%s,%s=%s", + def->os.machine, + "dump-guest-core", + virDomainMemDumpTypeToString(def->mem.dump_core)); + } + + return 0; +} + static char * qemuBuildSmpArgStr(const virDomainDefPtr def, qemuCapsPtr caps) @@ -4498,12 +4537,8 @@ qemuBuildCommandLine(virConnectPtr conn, } virCommandAddArg(cmd, "-S"); /* freeze CPU */ - /* This should *never* be NULL, since we always provide - * a machine in the capabilities data for QEMU. So this - * check is just here as a safety in case the unexpected - * happens */ - if (def->os.machine) - virCommandAddArgList(cmd, "-M", def->os.machine, NULL); + if (qemuBuildMachineArgStr(cmd, def, caps) < 0) + goto error; if (qemuBuildCpuArgStr(driver, def, emulator, caps, &ut, &cpu, &hasHwVirt, !!migrateFrom) < 0) @@ -8320,10 +8355,38 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, } if (STREQ(def->name, "")) VIR_FREE(def->name); - } else if (STREQ(arg, "-M")) { + } else if (STREQ(arg, "-M") || + STREQ(arg, "-machine")) { + char *params; WANT_VALUE(); - if (!(def->os.machine = strdup(val))) - goto no_memory; + params = strchr(val, ','); + if (params == NULL) { + if (!(def->os.machine = strdup(val))) + goto no_memory; + } else { + if (!(def->os.machine = strndup(val, params - val))) + goto no_memory; + + while(params++) { + /* prepared for more "-machine" parameters */ + char *tmp = params; + params = strchr(params, ','); + + if (STRPREFIX(tmp, "dump-guest-core=")) { + tmp += strlen("dump-guest-core="); + if (params) { + tmp = strndup(tmp, params - tmp); + if (tmp == NULL) + goto no_memory; + } + def->mem.dump_core = virDomainMemDumpTypeFromString(tmp); + if (def->mem.dump_core < 0) + def->mem.dump_core = VIR_DOMAIN_MEM_DUMP_DEFAULT; + if (params) + VIR_FREE(tmp); + } + } + } } else if (STREQ(arg, "-serial")) { WANT_VALUE(); if (STRNEQ(val, "none")) { diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index a3d9656..079aef8 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -846,7 +846,8 @@ mymain(void) QEMU_CAPS_VIRTIO_SCSI_PCI, QEMU_CAPS_BLOCKIO, QEMU_CAPS_SCSI_DISK_WWN, - QEMU_CAPS_SECCOMP_SANDBOX); + QEMU_CAPS_SECCOMP_SANDBOX, + QEMU_CAPS_DUMP_GUEST_CORE); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.7.12

On 09/20/2012 02:54 PM, Michal Privoznik wrote: I asked about this on qemu-devel and we really should do that. I'm planning on adding this (with '-machine' capabilities etc.), but as you said, that is an issue for separate patch. Yep, dunno what I was thinking about in here. It works, though =) Fixed. All fixed, checks are OK. Pushed now, thanks. Martin