Re: [libvirt] [PATCH 1/2] qemu: add a max_core setting to qemu.conf for core dump size

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.

> (* 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.

> +# 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...

> @@ -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]

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