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

[Martin Kletzander]

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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160712/5aace936/attachment-0001.sig>