On Wed, Aug 17, 2016 at 05:04:48PM -0400, John Ferlan wrote:

On 08/03/2016 11:31 AM, Daniel P. Berrange wrote:

...

Currently the QEMU processes inherit their core dump rlimit from libvirtd, which is really suboptimal. This change allows their limit to be directly controlled from qemu.conf instead. --- src/libvirt_private.syms | 2 ++ src/qemu/libvirtd_qemu.aug | 4 ++++ src/qemu/qemu.conf | 23 ++++++++++++++++++++++- src/qemu/qemu_conf.c | 17 +++++++++++++++++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_process.c | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/util/vircommand.c | 14 ++++++++++++++ src/util/vircommand.h | 1 + src/util/virprocess.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 1 + 11 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 419c33d..6dc2b23 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1389,6 +1389,7 @@ virCommandSetErrorFD; virCommandSetGID; virCommandSetInputBuffer; virCommandSetInputFD; +virCommandSetMaxCoreSize; virCommandSetMaxFiles; virCommandSetMaxMemLock; virCommandSetMaxProcesses; @@ -2199,6 +2200,7 @@ virProcessRunInMountNamespace; virProcessSchedPolicyTypeFromString; virProcessSchedPolicyTypeToString; virProcessSetAffinity; +virProcessSetMaxCoreSize; virProcessSetMaxFiles; virProcessSetMaxMemLock; virProcessSetMaxProcesses; diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 8bc23ba..9ec8250 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -22,6 +22,9 @@ module Libvirtd_qemu = let int_entry (kw:string) = [ key kw . value_sep . int_val ] let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ]

+ let unlimited_val = del /\"/ "\"" . store /unlimited/ . del /\"/ "\"" + let limits_entry (kw:string) = [ key kw . value_sep . unlimited_val ] | [ key kw . value_sep . int_val ] +

Is there no 'uulong_val'? At the very least uint? I don't know much about this aug stuff, so I defer to you on this.

There's no built-in types at all in augeas - we just define the grammar entirely ourselves.

...

(* Config entry grouped by function - same order as example config *) let vnc_entry = str_entry "vnc_listen" @@ -72,6 +75,7 @@ module Libvirtd_qemu = | bool_entry "set_process_name" | int_entry "max_processes" | int_entry "max_files" + | limits_entry "max_core" | str_entry "stdio_handler"

let device_entry = bool_entry "mac_filter" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 7964273..abf9938 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -401,7 +401,28 @@ #max_processes = 0 #max_files = 0

- +# If max_core is set to a positive integer, then QEMU will be +# permitted to create core dumps when it crashes, provided its +# RAM size is smaller than the limit set.

Providing a negative value will cause libvirtd startup failure... The way this is worded could lead one to believe they could provide a signed value.

Ok, i'll reword it.

...

+# Be warned that the core dump will include a full copy of the +# guest RAM, unless it has been disabled via the guest XML by +# setting: +# +# <memory dumpcore="off">...guest ram...</memory> +# +# If guest RAM is to be included, ensure the max_core limit +# is set to at least the size of the largest expected guest +# plus another 1GB for any QEMU host side memory mappings. +# +# As a special case it can be set to the string "unlimited" to +# to allow arbitrarily sized core dumps. +# +# By default the core dump size is set to 0 disabling all dumps +# +# Size is in bytes or string "unlimited" +# +#max_core = "unlimited"

Would it be overly complicated to alter max_core to be "unlimited" or a sized value? I would think it would be easier to type/see "5G" rather than do the math! or fat-finger or cut-n-paste wrong...

Currently our config file format is essentially python variable initialization syntax, so you can have unquoted numbers, or quoted strings. So we could not have a bare 5G - it would have to be input as a string which I think would be confusing - eg users would expect these to work: max_core = 5368709120 max_core = 5G but would in fact have to use max_core = 5368709120 max_core = "5G" we could solve this by extending the virConf parsing code to deal with bare sized values, but I don't fancy attacking that problem right now.

...

@@ -1105,6 +1110,15 @@ virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files) cmd->maxFiles = files; }

+void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes) +{ + if (!cmd || cmd->has_error) + return; + + cmd->maxCore = bytes;

Should this check if bytes == 0 before setting to true? If not, then another change probably would be necessary below... [1]

Yep, we can check for 0.

...

diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 09dd3c9..2b71445 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -914,6 +914,42 @@ virProcessSetMaxFiles(pid_t pid ATTRIBUTE_UNUSED, unsigned int files) } #endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_NOFILE)) */

+#if HAVE_SETRLIMIT && defined(RLIMIT_CORE) +int +virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes) +{ + struct rlimit rlim; + + rlim.rlim_cur = rlim.rlim_max = bytes; + if (pid == 0) { + if (setrlimit(RLIMIT_CORE, &rlim) < 0) { + virReportSystemError(errno, + _("cannot limit core file size to %llu"), + bytes); + return -1; + } + } else { + if (virProcessPrLimit(pid, RLIMIT_CORE, &rlim, NULL) < 0) { + virReportSystemError(errno, + _("cannot limit core file size " + "of process %lld to %llu"), + (long long int)pid, bytes); + return -1; + } + } + return 0; +} +#else /* ! (HAVE_SETRLIMIT && defined(RLIMIT_CORE)) */ +int +virProcessSetMaxCoreSize(pid_t pid ATTRIBUTE_UNUSED, + unsigned long long bytes ATTRIBUTE_UNUSED) +{

[1] Similar to other such functions should this have a :

if (!bytes) return 0;

and of course a removed ATTRIBUTE_UNUSED on bytes above...

Yep. Regards, 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 08/03/2016 11:31 AM, Daniel P. Berrange wrote:

We already have the ability to turn off dumping of guest RAM via the domain XML. This is not particularly useful though, as it is under control of the management application. What is needed is a way for the sysadmin to turn off guest RAM defaults globally, regardless of whether the mgmt app provides its own way to set this in the domain XML.

So this adds a 'dump_guest_core' option in /etc/libvirt/qemu.conf which defaults to false. ie guest RAM will never be included in the QEMU core dumps by default. This default is different from historical practice, but is considered to be more suitable as a default because

a) guest RAM can be huge and so inflicts a DOS on the host I/O subsystem when dumping core for QEMU crashes

b) guest RAM can contain alot of sensitive data belonging to the VM owner. This should not generally be copied around inside QEMU core dumps submitted to vendors for debugging

c) guest RAM contents are rarely useful in diagnosing QEMU crashes

Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 16 +++++++++++++--- src/qemu/qemu_command.c | 18 ++++++++++++------ src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + tests/qemuxml2argvtest.c | 4 ++++ 7 files changed, 35 insertions(+), 9 deletions(-)

Couldn't git am -3 this one, so I'm sure you have some merges to do in qemu_command.c (at least 90b42f0f and others afterwards from Michal) as well as qemuxml2argvtest.c...

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 9ec8250..c4ca77e 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -76,6 +76,7 @@ module Libvirtd_qemu = | int_entry "max_processes" | int_entry "max_files" | limits_entry "max_core" + | bool_entry "dump_guest_core" | str_entry "stdio_handler"

Strange alignment...

let device_entry = bool_entry "mac_filter" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index abf9938..67ab215 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -406,10 +406,10 @@ # RAM size is smaller than the limit set. # # Be warned that the core dump will include a full copy of the -# guest RAM, unless it has been disabled via the guest XML by -# setting: +# guest RAM, if the 'dump_guest_core' setting has been enabled, +# or if the guest XML contains # -# <memory dumpcore="off">...guest ram...</memory> +# <memory dumpcore="on">...guest ram...</memory> # # If guest RAM is to be included, ensure the max_core limit # is set to at least the size of the largest expected guest @@ -424,6 +424,16 @@ # #max_core = "unlimited"

+# Determine if guest RAM is included in QEMU core dumps. By +# default guest RAM will be excluded if a new enough QEMU is +# present. Setting this to '1' will force guest RAM to always +# be included in QEMU core dumps. +# +# This setting will be ignored if the guest XML has set the +# dumpcore attribute on the <memory> element. +# +#dump_guest_core = 1 + # mac_filter enables MAC addressed based filtering on bridge ports. # This currently requires ebtables to be installed. # diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 197537f..944e0b1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6927,6 +6927,7 @@ qemuBuildNameCommandLine(virCommandPtr cmd,

static int qemuBuildMachineCommandLine(virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, const virDomainDef *def, virQEMUCapsPtr qemuCaps) { @@ -6999,17 +7000,22 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, virTristateSwitchTypeToString(vmport)); }

- if (def->mem.dump_core) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) { + if (def->mem.dump_core) { + virBufferAsprintf(&buf, ",dump-guest-core=%s", + virTristateSwitchTypeToString(def->mem.dump_core)); + } else if (cfg->dumpGuestCore != true) {

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Will this even matter? Since there's a ternary below? IOW: There's no way to set this to "off" and I assume the by not providing it, it's off. Maybe the ternary doesn't matter... ACK with what appear to be obvious adjustments. John

+ virBufferAsprintf(&buf, ",dump-guest-core=%s", + cfg->dumpGuestCore ? "on" : "off"); + } + } else { + if (def->mem.dump_core) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("dump-guest-core is not available " "with this QEMU binary")); virBufferFreeAndReset(&buf); return -1; } - - virBufferAsprintf(&buf, ",dump-guest-core=%s", - virTristateSwitchTypeToString(def->mem.dump_core)); }

if (def->mem.nosharepages) { @@ -9365,7 +9371,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (enableFips) virCommandAddArg(cmd, "-enable-fips");

- if (qemuBuildMachineCommandLine(cmd, def, qemuCaps) < 0) + if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps) < 0) goto error;

if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps, !!migrateURI) < 0) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 45d039c..2cf879b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -649,6 +649,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; }

+ if (virConfGetValueBool(conf, "dump_guest_core", &cfg->dumpGuestCore) < 0) + goto cleanup; + if (virConfGetValueString(conf, "lock_manager", &cfg->lockManagerName) < 0) goto cleanup; if (virConfGetValueString(conf, "stdio_handler", &stdioHandler) < 0) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index b730202..c73d812 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -149,6 +149,7 @@ struct _virQEMUDriverConfig { unsigned int maxProcesses; unsigned int maxFiles; unsigned long long maxCore; + bool dumpGuestCore;

unsigned int maxQueuedJobs;

diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index a4797af..b4cc8d0 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -63,6 +63,7 @@ module Test_libvirtd_qemu = { "max_processes" = "0" } { "max_files" = "0" } { "max_core" = "unlimited" } +{ "dump_guest_core" = "1" } { "mac_filter" = "1" } { "relaxed_acs_check" = "1" } { "allow_disk_format_probing" = "1" } diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a5d51a8..0697a15 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -614,8 +614,12 @@ mymain(void) DO_TEST("machine-aliases2", QEMU_CAPS_KVM); DO_TEST("machine-core-on", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_DUMP_GUEST_CORE); + driver.config->dumpGuestCore = true; DO_TEST("machine-core-off", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_DUMP_GUEST_CORE); + driver.config->dumpGuestCore = false; + DO_TEST("machine-core-cfg-off", QEMU_CAPS_MACHINE_OPT, + QEMU_CAPS_DUMP_GUEST_CORE); DO_TEST_FAILURE("machine-core-on", NONE); DO_TEST_FAILURE("machine-core-on", QEMU_CAPS_MACHINE_OPT); DO_TEST("machine-usb-opt", QEMU_CAPS_MACHINE_OPT,