Re: [PATCH v2] qemu: add a max_core setting to qemu.conf for core dump size

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jul 12, 2016 at 10:56:53AM +0100, Daniel P. Berrange wrote:
On Tue, Jul 12, 2016 at 11:38:22AM +0200, Martin Kletzander wrote:
On Tue, Jul 12, 2016 at 10:12:36AM +0100, 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 controller from qemu.conf instead.

s/controller/controlled/

> ---
>
> Changed in v2:
>
> - Allow use of string "unlimited"
>
> src/libvirt_private.syms           |  2 ++
> src/qemu/libvirtd_qemu.aug         |  1 +
> src/qemu/qemu.conf                 | 16 +++++++++++++++-
> 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, 90 insertions(+), 1 deletion(-)
>

[...]

> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index 8bc23ba..a8edc2b 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -72,6 +72,7 @@ module Libvirtd_qemu =
>                  | bool_entry "set_process_name"
>                  | int_entry "max_processes"
>                  | int_entry "max_files"
> +                 | int_entry "max_core"

This should be expanded to allow "unlimited" as well.

Opps, yes.

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

This is not initialized anywhere, effectively making the limit default
to 0 IIUC.

Yes, that's correct as per the qemu.conf docs - we don't allow
core dumps by default, since they can be absolutely massive
and so have a significant impact on the host OS in terms of time
& I/O bandwidth required to dump the guest, as well as disk space
usage. So core dumps are an opt-in thing.


I must've missed the place where we forbade it then.

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

Shouldn't be this set to RLIM_INFINITY if "unlimited" was requested?
Not taht ULLONG_MAX would not be enough, it's just that it's rlim_t and
it would be nicer to use it as described.

IIUC, you're effectively saying that we should do

 if (bytes == ULLONG_MAX)
    rlim_max = RLIM_INFINITY;
 else
    rlim_max = bytes;

RLIM_INFINITY is just an integer constant - if that's not the same
as ULONG_MAX, then we ought to deal with that right up front a time
of parsing in QEMU. eg add a virConfGetValueRLin() that returns an
rlim_t value so we get range checking.


Well, my idea was to store it differently, mostly because of the
default/0 difference, but if default and 0 are the same, then I guess it
would be just making the code more complex.  I am also worried that if
rlim_t can't fir the whole ullong_max (e.g. without glibc wrappers on
older kernel, then it might wrap weirdly.  However reading up on the
prlimit(2) it looks like we're fine with glibc.  Not sure about uClibc
and others, though.


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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]