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

[Daniel P. Berrange]

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

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


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