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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
