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

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 :| -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list