[libvirt] [PATCH 0/2] Support core file limit settings for QEMU

Currently QEMU inherits core size limits from libvirtd which is rather inconvenient Daniel P. Berrange (2): conf: parse integers into long long, instead of long qemu: add a max_core setting to qemu.conf for core dump size daemon/libvirtd-config.c | 6 +-- src/libvirt_private.syms | 2 + src/locking/lock_daemon_config.c | 4 +- src/locking/lock_driver_lockd.c | 4 +- src/locking/lock_driver_sanlock.c | 6 +-- src/lxc/lxc_conf.c | 6 +-- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 12 +++++ src/qemu/qemu_conf.c | 9 ++-- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_process.c | 2 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/util/vircommand.c | 14 +++++ src/util/vircommand.h | 1 + src/util/virconf.c | 22 ++++---- src/util/virconf.h | 6 +-- src/util/virprocess.c | 35 +++++++++++++ src/util/virprocess.h | 1 + src/xenconfig/xen_common.c | 8 +-- tests/Makefile.am | 5 ++ tests/libvirtdconftest.c | 4 +- tests/virconftest.c | 105 +++++++++++++++++++++++++++++++++++++ 22 files changed, 220 insertions(+), 36 deletions(-) create mode 100644 tests/virconftest.c -- 2.1.0

When parsing integer values, we only used 'long' data type in the virConfValue struct. This is insufficiently large to deal with things like guest memory sizes on 32-bit platforms which are using PAE for addressing > 4 GB of RAM. --- daemon/libvirtd-config.c | 6 +-- src/locking/lock_daemon_config.c | 4 +- src/locking/lock_driver_lockd.c | 4 +- src/locking/lock_driver_sanlock.c | 6 +-- src/lxc/lxc_conf.c | 6 +-- src/qemu/qemu_conf.c | 6 +-- src/util/virconf.c | 22 ++++---- src/util/virconf.h | 6 +-- src/xenconfig/xen_common.c | 8 +-- tests/Makefile.am | 5 ++ tests/libvirtdconftest.c | 4 +- tests/virconftest.c | 105 ++++++++++++++++++++++++++++++++++++++ 12 files changed, 146 insertions(+), 36 deletions(-) create mode 100644 tests/virconftest.c diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c index 3694455..9344ccc 100644 --- a/daemon/libvirtd-config.c +++ b/daemon/libvirtd-config.c @@ -151,8 +151,8 @@ checkType(virConfValuePtr p, const char *filename, do { \ virConfValuePtr p = virConfGetValue(conf, #var_name); \ if (p) { \ - if (p->type != VIR_CONF_ULONG && \ - checkType(p, filename, #var_name, VIR_CONF_LONG) < 0) \ + if (p->type != VIR_CONF_ULLONG && \ + checkType(p, filename, #var_name, VIR_CONF_LLONG) < 0) \ goto error; \ data->var_name = p->l; \ } \ @@ -163,7 +163,7 @@ checkType(virConfValuePtr p, const char *filename, do { \ virConfValuePtr p = virConfGetValue(conf, #var_name); \ if (p) { \ - if (checkType(p, filename, #var_name, VIR_CONF_ULONG) < 0) \ + if (checkType(p, filename, #var_name, VIR_CONF_ULLONG) < 0) \ goto error; \ data->var_name = p->l; \ } \ diff --git a/src/locking/lock_daemon_config.c b/src/locking/lock_daemon_config.c index 8a6d18f..20718a2 100644 --- a/src/locking/lock_daemon_config.c +++ b/src/locking/lock_daemon_config.c @@ -75,7 +75,7 @@ checkType(virConfValuePtr p, const char *filename, do { \ virConfValuePtr p = virConfGetValue(conf, #var_name); \ if (p) { \ - if (p->type != VIR_CONF_ULONG && \ + if (p->type != VIR_CONF_ULLONG && \ checkType(p, filename, #var_name, VIR_CONF_LONG) < 0) \ goto error; \ data->var_name = p->l; \ @@ -87,7 +87,7 @@ checkType(virConfValuePtr p, const char *filename, do { \ virConfValuePtr p = virConfGetValue(conf, #var_name); \ if (p) { \ - if (checkType(p, filename, #var_name, VIR_CONF_ULONG) < 0) \ + if (checkType(p, filename, #var_name, VIR_CONF_ULLONG) < 0) \ goto error; \ data->var_name = p->l; \ } \ diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 72a4a0c..59cdd45 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -108,7 +108,7 @@ static int virLockManagerLockDaemonLoadConfig(const char *configFile) } p = virConfGetValue(conf, "auto_disk_leases"); - CHECK_TYPE("auto_disk_leases", VIR_CONF_ULONG); + CHECK_TYPE("auto_disk_leases", VIR_CONF_ULLONG); if (p) driver->autoDiskLease = p->l; p = virConfGetValue(conf, "file_lockspace_dir"); @@ -142,7 +142,7 @@ static int virLockManagerLockDaemonLoadConfig(const char *configFile) } p = virConfGetValue(conf, "require_lease_for_disks"); - CHECK_TYPE("require_lease_for_disks", VIR_CONF_ULONG); + CHECK_TYPE("require_lease_for_disks", VIR_CONF_ULLONG); if (p) driver->requireLeaseForDisks = p->l; else diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index dbe79bc..88631df 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -127,7 +127,7 @@ static int virLockManagerSanlockLoadConfig(const char *configFile) } p = virConfGetValue(conf, "auto_disk_leases"); - CHECK_TYPE("auto_disk_leases", VIR_CONF_ULONG); + CHECK_TYPE("auto_disk_leases", VIR_CONF_ULLONG); if (p) driver->autoDiskLease = p->l; p = virConfGetValue(conf, "disk_lease_dir"); @@ -141,11 +141,11 @@ static int virLockManagerSanlockLoadConfig(const char *configFile) } p = virConfGetValue(conf, "host_id"); - CHECK_TYPE("host_id", VIR_CONF_ULONG); + CHECK_TYPE("host_id", VIR_CONF_ULLONG); if (p) driver->hostID = p->l; p = virConfGetValue(conf, "require_lease_for_disks"); - CHECK_TYPE("require_lease_for_disks", VIR_CONF_ULONG); + CHECK_TYPE("require_lease_for_disks", VIR_CONF_ULLONG); if (p) driver->requireLeaseForDisks = p->l; else diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index d1a3be5..955a19d 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -270,7 +270,7 @@ virLXCLoadDriverConfig(virLXCDriverConfigPtr cfg, } p = virConfGetValue(conf, "log_with_libvirtd"); - CHECK_TYPE("log_with_libvirtd", VIR_CONF_ULONG); + CHECK_TYPE("log_with_libvirtd", VIR_CONF_ULLONG); if (p) cfg->log_libvirtd = p->l; p = virConfGetValue(conf, "security_driver"); @@ -283,11 +283,11 @@ virLXCLoadDriverConfig(virLXCDriverConfigPtr cfg, } p = virConfGetValue(conf, "security_default_confined"); - CHECK_TYPE("security_default_confined", VIR_CONF_ULONG); + CHECK_TYPE("security_default_confined", VIR_CONF_ULLONG); if (p) cfg->securityDefaultConfined = p->l; p = virConfGetValue(conf, "security_require_confined"); - CHECK_TYPE("security_require_confined", VIR_CONF_ULONG); + CHECK_TYPE("security_require_confined", VIR_CONF_ULLONG); if (p) cfg->securityRequireConfined = p->l; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 2cf3905..bca05c9 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -475,19 +475,19 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, #define GET_VALUE_LONG(NAME, VAR) \ p = virConfGetValue(conf, NAME); \ - CHECK_TYPE_ALT(NAME, VIR_CONF_LONG, VIR_CONF_ULONG); \ + CHECK_TYPE_ALT(NAME, VIR_CONF_LLONG, VIR_CONF_ULLONG); \ if (p) \ VAR = p->l; #define GET_VALUE_ULONG(NAME, VAR) \ p = virConfGetValue(conf, NAME); \ - CHECK_TYPE(NAME, VIR_CONF_ULONG); \ + CHECK_TYPE(NAME, VIR_CONF_ULLONG);\ if (p) \ VAR = p->l; #define GET_VALUE_BOOL(NAME, VAR) \ p = virConfGetValue(conf, NAME); \ - CHECK_TYPE(NAME, VIR_CONF_ULONG); \ + CHECK_TYPE(NAME, VIR_CONF_ULLONG);\ if (p) \ VAR = p->l != 0; diff --git a/src/util/virconf.c b/src/util/virconf.c index 01e5a6a..9182b17 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -82,8 +82,8 @@ struct _virConfParserCtxt { VIR_ENUM_IMPL(virConf, VIR_CONF_LAST, "*unexpected*", - "long", - "unsigned long", + "long long", + "unsigned long long", "string", "list"); @@ -273,9 +273,9 @@ virConfSaveValue(virBufferPtr buf, virConfValuePtr val) switch (val->type) { case VIR_CONF_NONE: return -1; - case VIR_CONF_LONG: - case VIR_CONF_ULONG: - virBufferAsprintf(buf, "%ld", val->l); + case VIR_CONF_LLONG: + case VIR_CONF_ULLONG: + virBufferAsprintf(buf, "%lld", val->l); break; case VIR_CONF_STRING: if (strchr(val->str, '\n') != NULL) { @@ -346,7 +346,7 @@ virConfSaveEntry(virBufferPtr buf, virConfEntryPtr cur) ************************************************************************/ /** - * virConfParseLong: + * virConfParseLLong: * @ctxt: the parsing context * @val: the result * @@ -355,9 +355,9 @@ virConfSaveEntry(virBufferPtr buf, virConfEntryPtr cur) * Returns 0 in case of success and -1 in case of error */ static int -virConfParseLong(virConfParserCtxtPtr ctxt, long *val) +virConfParseLLong(virConfParserCtxtPtr ctxt, long long *val) { - long l = 0; + long long l = 0; int neg = 0; if (CUR == '-') { @@ -467,7 +467,7 @@ virConfParseValue(virConfParserCtxtPtr ctxt) virConfValuePtr ret, lst = NULL, tmp, prev; virConfType type = VIR_CONF_NONE; char *str = NULL; - long l = 0; + long long l = 0; SKIP_BLANKS; if (ctxt->cur >= ctxt->end) { @@ -536,8 +536,8 @@ virConfParseValue(virConfParserCtxtPtr ctxt) _("numbers not allowed in VMX format")); return NULL; } - type = (c_isdigit(CUR) || CUR == '+') ? VIR_CONF_ULONG : VIR_CONF_LONG; - if (virConfParseLong(ctxt, &l) < 0) + type = (c_isdigit(CUR) || CUR == '+') ? VIR_CONF_ULLONG : VIR_CONF_LLONG; + if (virConfParseLLong(ctxt, &l) < 0) return NULL; } else { virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting a value")); diff --git a/src/util/virconf.h b/src/util/virconf.h index 2b4b943..971bb5e 100644 --- a/src/util/virconf.h +++ b/src/util/virconf.h @@ -33,8 +33,8 @@ */ typedef enum { VIR_CONF_NONE = 0, /* undefined */ - VIR_CONF_LONG, /* a long int */ - VIR_CONF_ULONG, /* an unsigned long int */ + VIR_CONF_LLONG, /* a long long int */ + VIR_CONF_ULLONG, /* an unsigned long long int */ VIR_CONF_STRING, /* a string */ VIR_CONF_LIST, /* a list */ VIR_CONF_LAST, /* sentinel */ @@ -61,7 +61,7 @@ typedef virConfValue *virConfValuePtr; struct _virConfValue { virConfType type; /* the virConfType */ virConfValuePtr next; /* next element if in a list */ - long l; /* long integer */ + long long l; /* long long integer */ char *str; /* pointer to 0 terminated string */ virConfValuePtr list; /* list of a list */ }; diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 5f8eb71..f53e137 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -57,7 +57,7 @@ xenConfigGetBool(virConfPtr conf, return 0; } - if (val->type == VIR_CONF_ULONG) { + if (val->type == VIR_CONF_ULLONG) { *value = val->l ? 1 : 0; } else if (val->type == VIR_CONF_STRING) { *value = STREQ(val->str, "1") ? 1 : 0; @@ -87,7 +87,7 @@ xenConfigGetULong(virConfPtr conf, return 0; } - if (val->type == VIR_CONF_ULONG) { + if (val->type == VIR_CONF_ULLONG) { *value = val->l; } else if (val->type == VIR_CONF_STRING) { if (virStrToLong_ul(val->str, NULL, 10, value) < 0) { @@ -121,7 +121,7 @@ xenConfigGetULongLong(virConfPtr conf, return 0; } - if (val->type == VIR_CONF_ULONG) { + if (val->type == VIR_CONF_ULLONG) { *value = val->l; } else if (val->type == VIR_CONF_STRING) { if (virStrToLong_ull(val->str, NULL, 10, value) < 0) { @@ -275,7 +275,7 @@ xenConfigSetInt(virConfPtr conf, const char *setting, long long l) if (VIR_ALLOC(value) < 0) return -1; - value->type = VIR_CONF_LONG; + value->type = VIR_CONF_LLONG; value->next = NULL; value->l = l; diff --git a/tests/Makefile.am b/tests/Makefile.am index 9277c13..05ee574 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -148,6 +148,7 @@ EXTRA_DIST = \ test_helpers = commandhelper ssh test_conf test_programs = virshtest sockettest \ nodeinfotest virbuftest \ + virconftest \ commandtest seclabeltest \ virhashtest \ viratomictest \ @@ -1128,6 +1129,10 @@ virbuftest_SOURCES = \ virbuftest.c testutils.h testutils.c virbuftest_LDADD = $(LDADDS) +virconftest_SOURCES = \ + virconftest.c testutils.h testutils.c +virconftest_LDADD = $(LDADDS) + virhashtest_SOURCES = \ virhashtest.c virhashdata.h testutils.h testutils.c virhashtest_LDADD = $(LDADDS) diff --git a/tests/libvirtdconftest.c b/tests/libvirtdconftest.c index d589d51..3c4b3cd 100644 --- a/tests/libvirtdconftest.c +++ b/tests/libvirtdconftest.c @@ -65,7 +65,7 @@ munge_param(const char *datain, if (c_isspace(*tmp)) continue; if (c_isdigit(*tmp)) { - *type = VIR_CONF_ULONG; + *type = VIR_CONF_ULLONG; replace = "\"foo\""; } else if (*tmp == '[') { *type = VIR_CONF_LIST; @@ -130,7 +130,7 @@ testCorrupt(const void *opaque) #endif switch (type) { - case VIR_CONF_ULONG: + case VIR_CONF_ULLONG: if (!strstr(err->message, "invalid type: got string; expected unsigned long") && !strstr(err->message, "invalid type: got string; expected long")) { VIR_DEBUG("Wrong error for long: '%s'", diff --git a/tests/virconftest.c b/tests/virconftest.c new file mode 100644 index 0000000..b481c92 --- /dev/null +++ b/tests/virconftest.c @@ -0,0 +1,105 @@ +#include <config.h> + +#include "testutils.h" +#include "virconf.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +static int testConfDataTypes(const void *data ATTRIBUTE_UNUSED) +{ + static const char *confdata = + "longlongval = -9223372036854775806\n" + "ulonglongval = 18446744073709551615\n" + "stringval = \"The annoyed programmer spent the whole day shaving yaks\"\n" + "listval = [\"Hello\", -9223372036854775806, 18446744073709551615, \"World\"]\n"; + virConfPtr conf; + virConfValuePtr val; + int ret = -1; + + if (!(conf = virConfReadMem(confdata, + strlen(confdata), + 0))) + goto cleanup; + +#define GET_SETTING(setting) \ + if (!(val = virConfGetValue(conf, setting))) { \ + fprintf(stderr, "Missing %s entry\n", setting); \ + goto cleanup; \ + } + +#define CHECK_INT(datatype, field, value, format) \ + if (val->type != datatype) { \ + fprintf(stderr, "Expecting type %d, but got type %d\n", \ + datatype, val->type); \ + goto cleanup; \ + } \ + if (val->field != value) { \ + fprintf(stderr, "Expecting " format " but got " format "\n", \ + value, val->field); \ + goto cleanup; \ + } + +#define CHECK_STRING(value) \ + if (val->type != VIR_CONF_STRING) { \ + fprintf(stderr, "Expecting type %d, but got type %d\n", \ + VIR_CONF_STRING, val->type); \ + goto cleanup; \ + } \ + if (STRNEQ(val->str, value)) { \ + fprintf(stderr, "Expecting '%s' but got '%s'\n", \ + value, val->str); \ + goto cleanup; \ + } + +#define CHECK_LIST() \ + if (val->type != VIR_CONF_LIST) { \ + fprintf(stderr, "Expecting type %d, but got type %d\n", \ + VIR_CONF_LIST, val->type); \ + goto cleanup; \ + } \ + + GET_SETTING("longlongval"); + CHECK_INT(VIR_CONF_LLONG, l, -9223372036854775806LL, "%lld"); + + GET_SETTING("ulonglongval"); + CHECK_INT(VIR_CONF_ULLONG, l, 18446744073709551615ULL, "%llu"); + + GET_SETTING("stringval"); + CHECK_STRING("The annoyed programmer spent the whole day shaving yaks"); + + GET_SETTING("listval"); + CHECK_LIST(); + + val = val->list; + CHECK_STRING("Hello"); + val = val->next; + CHECK_INT(VIR_CONF_LLONG, l, -9223372036854775806LL, "%lld"); + val = val->next; + CHECK_INT(VIR_CONF_ULLONG, l, 18446744073709551615ULL, "%llu"); + val = val->next; + CHECK_STRING("World"); + + ret = 0; + cleanup: + virConfFree(conf); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + +#define DO_TEST(msg, cb) \ + do { \ + if (virtTestRun("Conf: " msg, cb, NULL) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST("Data types", testConfDataTypes); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 2.1.0

On 03/18/2015 08:36 AM, Daniel P. Berrange wrote:
When parsing integer values, we only used 'long' data type in the virConfValue struct. This is insufficiently large to deal with things like guest memory sizes on 32-bit platforms which are using PAE for addressing > 4 GB of RAM. --- daemon/libvirtd-config.c | 6 +-- src/locking/lock_daemon_config.c | 4 +- src/locking/lock_driver_lockd.c | 4 +- src/locking/lock_driver_sanlock.c | 6 +-- src/lxc/lxc_conf.c | 6 +-- src/qemu/qemu_conf.c | 6 +-- src/util/virconf.c | 22 ++++---- src/util/virconf.h | 6 +-- src/xenconfig/xen_common.c | 8 +-- tests/Makefile.am | 5 ++ tests/libvirtdconftest.c | 4 +- tests/virconftest.c | 105 ++++++++++++++++++++++++++++++++++++++ 12 files changed, 146 insertions(+), 36 deletions(-) create mode 100644 tests/virconftest.c
Beyond the concept of typing in that many numbers which boggles the mind and eyes... A couple of notes: virConfParseLLong() - comment needs to be updated from "Parse one long int value" to "long long" virConfSaveValue() - should the "%llu" be separate from "%lld", since now we're talking memory values? That leads to the more general question/issue of whether we virConfParseValue() - Similarly, should the virConfParseLLong have a virConfParseULLong? Yeah - I know, hard to imagine typing that many digits *and* being correct! Need to spend a long time shaving yaks and counting hairs to get that large! testCorrupt() - The error message "invalid type: got string; expected unsigned long" (and just "long") - Should they change to "unsigned long long" and "long long" respectively? ACK - unless there's something someone believes "has" to fit within the LONG/ULONG space. John

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. --- src/libvirt_private.syms | 2 ++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 12 ++++++++++++ src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_process.c | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 1 + src/util/vircommand.c | 14 ++++++++++++++ src/util/vircommand.h | 1 + src/util/virprocess.c | 35 +++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 1 + 11 files changed, 74 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ca3520d..7446357 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1240,6 +1240,7 @@ virCommandSetErrorFD; virCommandSetGID; virCommandSetInputBuffer; virCommandSetInputFD; +virCommandSetMaxCoreSize; virCommandSetMaxFiles; virCommandSetMaxMemLock; virCommandSetMaxProcesses; @@ -1951,6 +1952,7 @@ virProcessRunInMountNamespace; virProcessSchedPolicyTypeFromString; virProcessSchedPolicyTypeToString; virProcessSetAffinity; +virProcessSetMaxCoreSize; virProcessSetMaxFiles; virProcessSetMaxMemLock; virProcessSetMaxProcesses; diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 62951da..029a55a 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -71,6 +71,7 @@ module Libvirtd_qemu = | bool_entry "set_process_name" | int_entry "max_processes" | int_entry "max_files" + | int_entry "max_core" let device_entry = bool_entry "mac_filter" | bool_entry "relaxed_acs_check" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 1c589a2..12e4326 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -390,6 +390,18 @@ #max_processes = 0 #max_files = 0 +# If max_core is set to a positive integer, then QEMU will be +# permitted to create core dumps when it crashes, provided its +# RAM size is smaller than the limit set. Be warned that the +# core dump will include a full copy of the guest RAM, so if +# the largest guest is 32 GB in size, the max_core limit will +# have to be at least 33/34 GB to allow enough overhead. +# +# By default it will inherit core limit from libvirtd, which +# is usually set to 0 by systemd/init. +# +# Size is in bytes +#max_core = 0 # mac_filter enables MAC addressed based filtering on bridge ports. diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index bca05c9..97bb8b8 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -770,6 +770,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_BOOL("set_process_name", cfg->setProcessName); GET_VALUE_ULONG("max_processes", cfg->maxProcesses); GET_VALUE_ULONG("max_files", cfg->maxFiles); + GET_VALUE_ULONG("max_core", cfg->maxCore); + if (virConfGetValue(conf, "max_core")) + cfg->setMaxCore = true; GET_VALUE_STR("lock_manager", cfg->lockManagerName); diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index cb01fb6..ac7a1bf 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -143,6 +143,8 @@ struct _virQEMUDriverConfig { int maxProcesses; int maxFiles; + bool setMaxCore; + unsigned long long maxCore; int maxQueuedJobs; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 37cdb8f..995d6ef 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4731,6 +4731,8 @@ int qemuProcessStart(virConnectPtr conn, virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData); virCommandSetMaxProcesses(cmd, cfg->maxProcesses); virCommandSetMaxFiles(cmd, cfg->maxFiles); + if (cfg->setMaxCore) + virCommandSetMaxCoreSize(cmd, cfg->maxCore); virCommandSetUmask(cmd, 0x002); VIR_DEBUG("Setting up security labelling"); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index fc4935b..9bb3683 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -61,6 +61,7 @@ module Test_libvirtd_qemu = { "set_process_name" = "1" } { "max_processes" = "0" } { "max_files" = "0" } +{ "max_core" = "0" } { "mac_filter" = "1" } { "relaxed_acs_check" = "1" } { "allow_disk_format_probing" = "1" } diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 648f5ed..1982401 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -123,6 +123,8 @@ struct _virCommand { unsigned long long maxMemLock; unsigned int maxProcesses; unsigned int maxFiles; + bool setMaxCore; + unsigned long long maxCore; uid_t uid; gid_t gid; @@ -686,6 +688,9 @@ virExec(virCommandPtr cmd) goto fork_error; if (virProcessSetMaxFiles(0, cmd->maxFiles) < 0) goto fork_error; + if (cmd->setMaxCore && + virProcessSetMaxCoreSize(0, cmd->maxCore) < 0) + goto fork_error; if (cmd->hook) { VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque); @@ -1108,6 +1113,15 @@ virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files) cmd->maxFiles = files; } +void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes) +{ + if (!cmd || cmd->has_error) + return; + + cmd->maxCore = bytes; + cmd->setMaxCore = true; +} + void virCommandSetUmask(virCommandPtr cmd, int mask) { if (!cmd || cmd->has_error) diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 198da2f..4f2874d 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -75,6 +75,7 @@ void virCommandSetUID(virCommandPtr cmd, uid_t uid); void virCommandSetMaxMemLock(virCommandPtr cmd, unsigned long long bytes); void virCommandSetMaxProcesses(virCommandPtr cmd, unsigned int procs); void virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files); +void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes); void virCommandSetUmask(virCommandPtr cmd, int umask); void virCommandClearCaps(virCommandPtr cmd); diff --git a/src/util/virprocess.c b/src/util/virprocess.c index aa00a99..02d91d9 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -825,6 +825,41 @@ 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; + if (pid == 0) { + if (setrlimit(RLIMIT_CORE, &rlim) < 0) { + virReportSystemError(errno, + _("cannot limit core file size to %llu"), + bytes); + return -1; + } + } else { + if (virProcessPrLimit(pid, RLIMIT_CORE, &rlim) < 0) { + virReportSystemError(errno, + _("cannot limit core file size " + "of process %lld to %llu"), + (long long int)pid, bytes); + return -1; + } + } + return 0; +} +#else /* ! (HAVE_SETRLIMIT && defined(RLIMIT_CORE)) */ +int +virProcessSetMaxCoreSize(pid_t pid ATTRIBUTE_UNUSED, unsigned long long bytes) +{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return -1; +} +#endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_CORE)) */ + + #ifdef __linux__ /* * Port of code from polkitunixprocess.c under terms diff --git a/src/util/virprocess.h b/src/util/virprocess.h index c812882..cd7b7a2 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -75,6 +75,7 @@ int virProcessSetNamespaces(size_t nfdlist, int virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes); int virProcessSetMaxProcesses(pid_t pid, unsigned int procs); int virProcessSetMaxFiles(pid_t pid, unsigned int files); +int virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes); /* Callback to run code within the mount namespace tied to the given * pid. This function must use only async-signal-safe functions, as -- 2.1.0

On 03/18/2015 08:36 AM, 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. --- src/libvirt_private.syms | 2 ++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 12 ++++++++++++ src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_process.c | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 1 + src/util/vircommand.c | 14 ++++++++++++++ src/util/vircommand.h | 1 + src/util/virprocess.c | 35 +++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 1 + 11 files changed, 74 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ca3520d..7446357 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1240,6 +1240,7 @@ virCommandSetErrorFD; virCommandSetGID; virCommandSetInputBuffer; virCommandSetInputFD; +virCommandSetMaxCoreSize; virCommandSetMaxFiles; virCommandSetMaxMemLock; virCommandSetMaxProcesses; @@ -1951,6 +1952,7 @@ virProcessRunInMountNamespace; virProcessSchedPolicyTypeFromString; virProcessSchedPolicyTypeToString; virProcessSetAffinity; +virProcessSetMaxCoreSize; virProcessSetMaxFiles; virProcessSetMaxMemLock; virProcessSetMaxProcesses; diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 62951da..029a55a 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -71,6 +71,7 @@ module Libvirtd_qemu = | bool_entry "set_process_name" | int_entry "max_processes" | int_entry "max_files" + | int_entry "max_core"
let device_entry = bool_entry "mac_filter" | bool_entry "relaxed_acs_check" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 1c589a2..12e4326 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -390,6 +390,18 @@ #max_processes = 0 #max_files = 0
+# If max_core is set to a positive integer, then QEMU will be +# permitted to create core dumps when it crashes, provided its +# RAM size is smaller than the limit set. Be warned that the +# core dump will include a full copy of the guest RAM, so if +# the largest guest is 32 GB in size, the max_core limit will +# have to be at least 33/34 GB to allow enough overhead. +# +# By default it will inherit core limit from libvirtd, which +# is usually set to 0 by systemd/init. +# +# Size is in bytes +#max_core = 0
It's too bad it cannot be a "sized" value... Reading 20G in a file for me is so much easier than the comparable byte value say nothing if we get to 128G, 1T, etc. (some day).
# mac_filter enables MAC addressed based filtering on bridge ports. diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index bca05c9..97bb8b8 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -770,6 +770,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_BOOL("set_process_name", cfg->setProcessName); GET_VALUE_ULONG("max_processes", cfg->maxProcesses); GET_VALUE_ULONG("max_files", cfg->maxFiles); + GET_VALUE_ULONG("max_core", cfg->maxCore); + if (virConfGetValue(conf, "max_core")) + cfg->setMaxCore = true;
Should we ensure cfg->maxCore >= 0 (or does that get magically done by one the many macros). Paranoia based on shared LL/LLU algorithm (and that my eyes/brain is tired). Which leads me down the path of - if 'max_core' is set to zero or doesn't exist, then cfg->maxCore would be zero - so is the 'setMaxCore' necessary.
GET_VALUE_STR("lock_manager", cfg->lockManagerName);
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index cb01fb6..ac7a1bf 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -143,6 +143,8 @@ struct _virQEMUDriverConfig {
int maxProcesses; int maxFiles; + bool setMaxCore; + unsigned long long maxCore;
int maxQueuedJobs;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 37cdb8f..995d6ef 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4731,6 +4731,8 @@ int qemuProcessStart(virConnectPtr conn, virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData); virCommandSetMaxProcesses(cmd, cfg->maxProcesses); virCommandSetMaxFiles(cmd, cfg->maxFiles); + if (cfg->setMaxCore) + virCommandSetMaxCoreSize(cmd, cfg->maxCore); virCommandSetUmask(cmd, 0x002);
VIR_DEBUG("Setting up security labelling"); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index fc4935b..9bb3683 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -61,6 +61,7 @@ module Test_libvirtd_qemu = { "set_process_name" = "1" } { "max_processes" = "0" } { "max_files" = "0" } +{ "max_core" = "0" } { "mac_filter" = "1" } { "relaxed_acs_check" = "1" } { "allow_disk_format_probing" = "1" } diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 648f5ed..1982401 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -123,6 +123,8 @@ struct _virCommand { unsigned long long maxMemLock; unsigned int maxProcesses; unsigned int maxFiles; + bool setMaxCore; + unsigned long long maxCore;
uid_t uid; gid_t gid; @@ -686,6 +688,9 @@ virExec(virCommandPtr cmd) goto fork_error; if (virProcessSetMaxFiles(0, cmd->maxFiles) < 0) goto fork_error; + if (cmd->setMaxCore && + virProcessSetMaxCoreSize(0, cmd->maxCore) < 0) + goto fork_error;
if (cmd->hook) { VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque); @@ -1108,6 +1113,15 @@ virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files) cmd->maxFiles = files; }
+void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes) +{ + if (!cmd || cmd->has_error) + return; + + cmd->maxCore = bytes; + cmd->setMaxCore = true; +} + void virCommandSetUmask(virCommandPtr cmd, int mask) { if (!cmd || cmd->has_error) diff --git a/src/util/vircommand.h b/src/util/vircommand.h index 198da2f..4f2874d 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -75,6 +75,7 @@ void virCommandSetUID(virCommandPtr cmd, uid_t uid); void virCommandSetMaxMemLock(virCommandPtr cmd, unsigned long long bytes); void virCommandSetMaxProcesses(virCommandPtr cmd, unsigned int procs); void virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files); +void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes); void virCommandSetUmask(virCommandPtr cmd, int umask);
void virCommandClearCaps(virCommandPtr cmd); diff --git a/src/util/virprocess.c b/src/util/virprocess.c index aa00a99..02d91d9 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -825,6 +825,41 @@ 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; + if (pid == 0) { + if (setrlimit(RLIMIT_CORE, &rlim) < 0) { + virReportSystemError(errno, + _("cannot limit core file size to %llu"), + bytes); + return -1; + } + } else { + if (virProcessPrLimit(pid, RLIMIT_CORE, &rlim) < 0) { + virReportSystemError(errno, + _("cannot limit core file size " + "of process %lld to %llu"), + (long long int)pid, bytes); + return -1; + } + } + return 0; +} +#else /* ! (HAVE_SETRLIMIT && defined(RLIMIT_CORE)) */ +int +virProcessSetMaxCoreSize(pid_t pid ATTRIBUTE_UNUSED, unsigned long long bytes)
Aren't they both ATTRIBUTE_UNUSED In general an ACK with at least this change... I just keep wondering what happens if someone provides -1 for the value... John
+{ + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform")); + return -1; +} +#endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_CORE)) */ + + #ifdef __linux__ /* * Port of code from polkitunixprocess.c under terms diff --git a/src/util/virprocess.h b/src/util/virprocess.h index c812882..cd7b7a2 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -75,6 +75,7 @@ int virProcessSetNamespaces(size_t nfdlist, int virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes); int virProcessSetMaxProcesses(pid_t pid, unsigned int procs); int virProcessSetMaxFiles(pid_t pid, unsigned int files); +int virProcessSetMaxCoreSize(pid_t pid, unsigned long long bytes);
/* Callback to run code within the mount namespace tied to the given * pid. This function must use only async-signal-safe functions, as

On Mon, Mar 23, 2015 at 08:43:31PM -0400, John Ferlan wrote:
On 03/18/2015 08:36 AM, 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. --- src/libvirt_private.syms | 2 ++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 12 ++++++++++++ src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_process.c | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 1 + src/util/vircommand.c | 14 ++++++++++++++ src/util/vircommand.h | 1 + src/util/virprocess.c | 35 +++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 1 + 11 files changed, 74 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ca3520d..7446357 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1240,6 +1240,7 @@ virCommandSetErrorFD; virCommandSetGID; virCommandSetInputBuffer; virCommandSetInputFD; +virCommandSetMaxCoreSize; virCommandSetMaxFiles; virCommandSetMaxMemLock; virCommandSetMaxProcesses; @@ -1951,6 +1952,7 @@ virProcessRunInMountNamespace; virProcessSchedPolicyTypeFromString; virProcessSchedPolicyTypeToString; virProcessSetAffinity; +virProcessSetMaxCoreSize; virProcessSetMaxFiles; virProcessSetMaxMemLock; virProcessSetMaxProcesses; diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 62951da..029a55a 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -71,6 +71,7 @@ module Libvirtd_qemu = | bool_entry "set_process_name" | int_entry "max_processes" | int_entry "max_files" + | int_entry "max_core"
let device_entry = bool_entry "mac_filter" | bool_entry "relaxed_acs_check" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 1c589a2..12e4326 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -390,6 +390,18 @@ #max_processes = 0 #max_files = 0
+# If max_core is set to a positive integer, then QEMU will be +# permitted to create core dumps when it crashes, provided its +# RAM size is smaller than the limit set. Be warned that the +# core dump will include a full copy of the guest RAM, so if +# the largest guest is 32 GB in size, the max_core limit will +# have to be at least 33/34 GB to allow enough overhead. +# +# By default it will inherit core limit from libvirtd, which +# is usually set to 0 by systemd/init. +# +# Size is in bytes +#max_core = 0
It's too bad it cannot be a "sized" value... Reading 20G in a file for me is so much easier than the comparable byte value say nothing if we get to 128G, 1T, etc. (some day).
Having the option as a string would also allow non-integer values, like "unlimited". Jan

On Tue, Mar 24, 2015 at 08:26:12 +0100, Ján Tomko wrote:
On Mon, Mar 23, 2015 at 08:43:31PM -0400, John Ferlan wrote:
On 03/18/2015 08:36 AM, 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. --- src/libvirt_private.syms | 2 ++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 12 ++++++++++++ src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_process.c | 2 ++ src/qemu/test_libvirtd_qemu.aug.in | 1 + src/util/vircommand.c | 14 ++++++++++++++ src/util/vircommand.h | 1 + src/util/virprocess.c | 35 +++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 1 + 11 files changed, 74 insertions(+)
...
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 1c589a2..12e4326 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -390,6 +390,18 @@ #max_processes = 0 #max_files = 0
+# If max_core is set to a positive integer, then QEMU will be +# permitted to create core dumps when it crashes, provided its +# RAM size is smaller than the limit set. Be warned that the +# core dump will include a full copy of the guest RAM, so if +# the largest guest is 32 GB in size, the max_core limit will +# have to be at least 33/34 GB to allow enough overhead. +# +# By default it will inherit core limit from libvirtd, which +# is usually set to 0 by systemd/init. +# +# Size is in bytes +#max_core = 0
It's too bad it cannot be a "sized" value... Reading 20G in a file for me is so much easier than the comparable byte value say nothing if we get to 128G, 1T, etc. (some day).
Having the option as a string would also allow non-integer values, like "unlimited".
I definitely vote for an option to set unlimited rather than having to specify a large number to achieve the same effect.
Jan
Peter
participants (4)
-
Daniel P. Berrange
-
John Ferlan
-
Ján Tomko
-
Peter Krempa