On Thu, Sep 20, 2012 at 10:58:13AM +0200, Martin Kletzander wrote:

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">

As a general rule we have avoided use of '-' in XML element/attribute names, so this ought to be 'dumpCore' IMHO

+ <choice> + <value>on</value> + <value>off</value> + </choice> + </attribute> + </optional> </element> <optional> <element name="currentMemory">

Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Thu, Sep 20, 2012 at 11:54:06AM +0200, Martin Kletzander wrote:

On 09/20/2012 11:32 AM, Daniel P. Berrange wrote:

...

On Thu, Sep 20, 2012 at 10:58:13AM +0200, Martin Kletzander wrote:

...

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">

As a general rule we have avoided use of '-' in XML element/attribute names, so this ought to be 'dumpCore' IMHO

I didn't realize that, OK. It's fixed, should I send v3 for review because of that?

No need to resend just for that. Just double check tests still pass Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 09/20/2012 02:54 PM, Michal Privoznik wrote:

On 20.09.2012 10:58, Martin Kletzander wrote:

...

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">

s/-c/C/ as Dan pointed out

Done and double checked with tests.

...

+ <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) {

s/</<=/ because we don't want to let users type in 'default'

Fixed.

...

+ virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid value for <memory> 'dump-core' attribute"));

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.

I did that except the function fitted on one line.

...

+ 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;

ACK if you fix those nits.

Michal

Thanks for the review. Nits fixed, syntax-check and check ran OK, it's pushed now. Martin

On 20.09.2012 10:58, Martin Kletzander wrote:

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);

I wonder if we should switch to -machine unconditionally on enough new qemu (option was introduced in 0.15) and leave -M just for backwards compatibility since qemu is (sort of) maintaining -M just because of us. But that'd be a separate patch anyway.

+ } 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));

This looks weird to me: s/%s,%s=%s/%s,dump-guest-core=%s/

+ } + + 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)

I guess qemu refuses to start with dump-guest-core=default but no one knows. s/</<=/

+ 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; }

ACK with nits fixed. Michal