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...
> # mac_filter enables MAC addressed based filtering on bridge ports.
> # This currently requires ebtables to be installed.
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index fa9d65e..45d039c 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -393,6 +393,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
> char **controllers = NULL;
> char **hugetlbfs = NULL;
> char **nvram = NULL;
> + char *corestr = NULL;
>
> /* Just check the file is readable before opening it, otherwise
> * libvirt emits an error.
> @@ -633,6 +634,21 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
> if (virConfGetValueUInt(conf, "max_files", &cfg->maxFiles) < 0)
> goto cleanup;
>
> + if (virConfGetValueType(conf, "max_core") == VIR_CONF_STRING) {
> + if (virConfGetValueString(conf, "max_core", &corestr) < 0)
> + goto cleanup;
> + if (STREQ(corestr, "unlimited")) {
> + cfg->maxCore = ULLONG_MAX;
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Unknown core size '%s'"),
> + corestr);
> + goto cleanup;
> + }
> + } else if (virConfGetValueULLong(conf, "max_core", &cfg->maxCore) < 0) {
> + goto cleanup;
> + }
> +
> if (virConfGetValueString(conf, "lock_manager", &cfg->lockManagerName) < 0)
> goto cleanup;
> if (virConfGetValueString(conf, "stdio_handler", &stdioHandler) < 0)
> @@ -715,6 +731,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
> virStringFreeList(controllers);
> virStringFreeList(hugetlbfs);
> virStringFreeList(nvram);
> + VIR_FREE(corestr);
> VIR_FREE(user);
> VIR_FREE(group);
> virConfFree(conf);
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 510cd9a..b730202 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -148,6 +148,7 @@ struct _virQEMUDriverConfig {
>
> unsigned int maxProcesses;
> unsigned int maxFiles;
> + unsigned long long maxCore;
>
> unsigned int maxQueuedJobs;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index f9efff9..2270fce 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5056,6 +5056,7 @@ qemuProcessLaunch(virConnectPtr conn,
> virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData);
> virCommandSetMaxProcesses(cmd, cfg->maxProcesses);
> virCommandSetMaxFiles(cmd, cfg->maxFiles);
> + virCommandSetMaxCoreSize(cmd, cfg->maxCore);
> virCommandSetUmask(cmd, 0x002);
>
> VIR_DEBUG("Setting up security labelling");
> diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
> index c4d4f19..a4797af 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -62,6 +62,7 @@ module Test_libvirtd_qemu =
> { "set_process_name" = "1" }
> { "max_processes" = "0" }
> { "max_files" = "0" }
> +{ "max_core" = "unlimited" }
> { "mac_filter" = "1" }
> { "relaxed_acs_check" = "1" }
> { "allow_disk_format_probing" = "1" }
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 3c67c90..2a59bd1 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -124,6 +124,8 @@ struct _virCommand {
> unsigned long long maxMemLock;
> unsigned int maxProcesses;
> unsigned int maxFiles;
> + bool setMaxCore;
> + unsigned long long maxCore;
>
> uid_t uid;
> gid_t gid;
> @@ -687,6 +689,9 @@ virExec(virCommandPtr cmd)
> goto fork_error;
> if (virProcessSetMaxFiles(0, cmd->maxFiles) < 0)
> goto fork_error;
> + if (cmd->setMaxCore &&
> + virProcessSetMaxCoreSize(0, cmd->maxCore) < 0)
> + goto fork_error;
>
> if (cmd->hook) {
> VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque);
> @@ -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]
> + cmd->setMaxCore = true;
> +}
> +
> void virCommandSetUmask(virCommandPtr cmd, int mask)
> {
> if (!cmd || cmd->has_error)
> diff --git a/src/util/vircommand.h b/src/util/vircommand.h
> index 44818ef..99dcdeb 100644
> --- a/src/util/vircommand.h
> +++ b/src/util/vircommand.h
> @@ -75,6 +75,7 @@ void virCommandSetUID(virCommandPtr cmd, uid_t uid);
> void virCommandSetMaxMemLock(virCommandPtr cmd, unsigned long long bytes);
> void virCommandSetMaxProcesses(virCommandPtr cmd, unsigned int procs);
> void virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files);
> +void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes);
> void virCommandSetUmask(virCommandPtr cmd, int umask);
>
> void virCommandClearCaps(virCommandPtr cmd);
> 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...
logic:
maxCore defaults to 0.
qemuProcessLaunch calls virCommandSetMaxCoreSize (setting setMaxCore)
in virExec if setMaxCore is true, then call virProcessSetMaxCoreSize
ACK - in principal - your call on the sized/scaled value, but I do think
this !bytes should be done...
John
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return -1;
> +}
> +#endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_CORE)) */
> +
> +
> #ifdef __linux__
> /*
> * Port of code from polkitunixprocess.c under terms
> diff --git a/src/util/virprocess.h b/src/util/virprocess.h
> index a7a1fe9..04e9802 100644
> --- a/src/util/virprocess.h
> +++ b/src/util/virprocess.h
> @@ -75,6 +75,7 @@ int virProcessSetNamespaces(size_t nfdlist,
> int virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes);
> int virProcessSetMaxProcesses(pid_t pid, unsigned int procs);
> int virProcessSetMaxFiles(pid_t pid, unsigned int files);
> +int virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes);
>
> int virProcessGetMaxMemLock(pid_t pid, unsigned long long *bytes);
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list