- Devel - libvirt List Archives

[libvirt] [PATCH] Add support for -enable-kqemu flag
by Daniel P. Berrange 18 Aug '10

18 Aug '10

Previously QEMU enabled KQEMU by default and had -no-kqemu. 0.11.x switched to requiring -enable-kqemu. 0.12.x dropped kqemu entirely. This patch adds support for -enable-kqemu so 0.11.x works. It replaces a huge set of if() with a switch() to make the code a bit more readable. This patch was inspired by the one John Lumby send to the list a while ago, but the way I've implemented it is more in line with the logic we have for KVM. http://www.redhat.com/archives/libvir-list/2010-July/msg00368.html * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Support -enable-kqemu --- src/qemu/qemu_conf.c | 84 +++++++++++++++++++++++++++++++++---------------- src/qemu/qemu_conf.h | 1 + 2 files changed, 57 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index fb85220..0ddf128 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1156,6 +1156,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, if (strstr(help, "-no-kqemu")) flags |= QEMUD_CMD_FLAG_KQEMU; + if (strstr(help, "-enable-kqemu")) + flags |= QEMUD_CMD_FLAG_ENABLE_KQEMU; if (strstr(help, "-no-kvm")) flags |= QEMUD_CMD_FLAG_KVM; if (strstr(help, "-enable-kvm")) @@ -3662,6 +3664,7 @@ int qemudBuildCommandLine(virConnectPtr conn, char boot[VIR_DOMAIN_BOOT_LAST]; struct utsname ut; int disableKQEMU = 0; + int enableKQEMU = 0; int disableKVM = 0; int enableKVM = 0; int qargc = 0, qarga = 0; @@ -3711,38 +3714,59 @@ int qemudBuildCommandLine(virConnectPtr conn, emulator = def->emulator; - /* Need to explicitly disable KQEMU if - * 1. Guest domain is 'qemu' - * 2. The qemu binary has the -no-kqemu flag - */ - if ((qemuCmdFlags & QEMUD_CMD_FLAG_KQEMU) && - def->virtType == VIR_DOMAIN_VIRT_QEMU) - disableKQEMU = 1; - - /* Need to explicitly disable KVM if - * 1. Guest domain is 'qemu' - * 2. The qemu binary has the -no-kvm flag + /* + * do not use boot=on for drives when not using KVM since this + * is not supported at all in upstream QEmu. */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_KVM) && - def->virtType == VIR_DOMAIN_VIRT_QEMU) { - disableKVM = 1; + (def->virtType == VIR_DOMAIN_VIRT_QEMU) && + (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_BOOT)) + qemuCmdFlags -= QEMUD_CMD_FLAG_DRIVE_BOOT; + + switch (def->virtType) { + case VIR_DOMAIN_VIRT_QEMU: + if (qemuCmdFlags & QEMUD_CMD_FLAG_KQEMU) + disableKQEMU = 1; + if (qemuCmdFlags & QEMUD_CMD_FLAG_KVM) + disableKVM = 1; + break; - /* - * do not use boot=on for drives when not using KVM since this - * is not supported at all in upstream QEmu. - */ - if (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_BOOT) - qemuCmdFlags -= QEMUD_CMD_FLAG_DRIVE_BOOT; - } + case VIR_DOMAIN_VIRT_KQEMU: + if (qemuCmdFlags & QEMUD_CMD_FLAG_KVM) + disableKVM = 1; - /* Should explicitly enable KVM if - * 1. Guest domain is 'kvm' - * 2. The qemu binary has the -enable-kvm flag - * NOTE: user must be responsible for loading the kvm modules - */ - if ((qemuCmdFlags & QEMUD_CMD_FLAG_ENABLE_KVM) && - def->virtType == VIR_DOMAIN_VIRT_KVM) - enableKVM = 1; + if (qemuCmdFlags & QEMUD_CMD_FLAG_ENABLE_KQEMU) { + enableKQEMU = 1; + } else if (!(qemuCmdFlags & QEMUD_CMD_FLAG_KQEMU)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("the QEMU binary %s does not support kqemu"), + emulator); + } + break; + + case VIR_DOMAIN_VIRT_KVM: + if (qemuCmdFlags & QEMUD_CMD_FLAG_KQEMU) + disableKQEMU = 1; + + if (qemuCmdFlags & QEMUD_CMD_FLAG_ENABLE_KVM) { + enableKVM = 1; + } else if (!(qemuCmdFlags & QEMUD_CMD_FLAG_KVM)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("the QEMU binary %s does not support kvm"), + emulator); + } + break; + + case VIR_DOMAIN_VIRT_XEN: + /* XXX better check for xenner */ + break; + + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("the QEMU binary %s does not support %s"), + emulator, virDomainVirtTypeToString(def->virtType)); + break; + } #define ADD_ARG_SPACE \ do { \ @@ -3857,6 +3881,10 @@ int qemudBuildCommandLine(virConnectPtr conn, if (disableKQEMU) ADD_ARG_LIT("-no-kqemu"); + else if (enableKQEMU) { + ADD_ARG_LIT("-enable-kqemu"); + ADD_ARG_LIT("-kernel-kqemu"); + } if (disableKVM) ADD_ARG_LIT("-no-kvm"); if (enableKVM) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1aa9d2e..4f9c714 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -92,6 +92,7 @@ enum qemud_cmd_flags { QEMUD_CMD_FLAG_PCI_CONFIGFD = (1LL << 36), /* pci-assign.configfd */ QEMUD_CMD_FLAG_NODEFCONFIG = (1LL << 37), /* -nodefconfig */ QEMUD_CMD_FLAG_BOOT_MENU = (1LL << 38), /* -boot menu=on support */ + QEMUD_CMD_FLAG_ENABLE_KQEMU = (1LL << 39), /* Is the -enable-kqemu flag available to "enable KQEMU full virtualization support" */ }; /* Main driver state */ -- 1.7.2.1

2 1

[libvirt] [PATCH] docs: mention domain <clock> improvements
by Eric Blake 18 Aug '10

18 Aug '10

Add documentation for features added a while ago. * docs/formatdomain.html.in (Time keeping): Update documentation of <clock> element to match 0.8.0 addition. --- I've had this sitting on my tree for a while; any suggestions for improvements, or should I at least check it in as-is so that we have a start on this documentation? docs/formatdomain.html.in | 76 ++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 71 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 5269cc5..f9a153b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -411,13 +411,18 @@ <pre> ... - <clock offset="localtime"/> + <clock offset="localtime"> + <timer name="rtc" tickpolicy="catchup" track="guest"> + <catchup threshold=123 slew=120 limit=10000/> + </timer> + <timer name="pit" tickpolicy="none"/> + </clock> ...</pre> <dl> <dt><code>clock</code></dt> <dd> - The <code>offset</code> attribute takes three possible + The <code>offset</code> attribute takes four possible values, allowing fine grained control over how the guest clock is synchronized to the host. NB, not all hypervisors support all modes. @@ -435,6 +440,7 @@ <dd> The guest clock will be synchronized to the requested timezone using the <code>timezone</code> attribute. + Since 0.7.7 </dd> <dt><code>variable</code></dt> <dd> @@ -444,14 +450,74 @@ The guest is free to adjust the RTC over time an expect that it will be honoured at next reboot. This is in contrast to 'utc' mode, where the RTC adjustments are - lost at each reboot. + lost at each reboot. Since 0.7.7 </dd> </dl> - NB, at time of writing, only QEMU supports the variable - clock mode, or custom timezones. + A <code>clock</code> may have zero or more + <code>timer</code>sub-elements. Since + 0.8.0 </dd> + <dt><code>timer</code></dt> + <dd> + + Each timer element requires a <code>name</code> attribute, + and has other optional attributes that depend on + the <code>name</code> specified. Various hypervisors + support different combinations of attributes. + + <dl> + <dt><code>name</code></dt> + <dd> + The <code>name</code> attribute selects which timer is + being modified, and can be one of "platform", "pit", + "rtc", "hpet", or "tsc". + </dd> + <dt><code>track</code></dt> + <dd> + The <code>track</code> attribute specifies what the timer + tracks, and can be "boot", "guest", or "wall". + Only valid for <code>name="rtc"</code> + or <code>name="platform"</code>. + </dd> + <dt><code>tickpolicy</code></dt> + <dd> + The <code>tickpolicy</code> attribute determines how + missed ticks in the guest are handled, and can be "delay", + "catchup", "merge", or "discard". If the policy is + "catchup", there can be further details in + the <code>catchup</code> sub-element. + <dl> + <dt><code>catchup</code></dt> + <dd> + The <code>catchup</code> element has three optional + attributes, each a positive integer. The attributes + are <code>threshold</code>, <code>slew</code>, + and <code>limit</code>. + </dd> + </dl> + </dd> + <dt><code>frequency</code></dt> + <dd> + The <code>frequency</code> attribute is an unsigned + integer specifying the frequency at + which <code>name="tsc"</code> runs. + </dd> + <dt><code>mode</code></dt> + <dd> + The <code>mode</code> attribute controls how + the <code>name="tsc"</code> timer is managed, and can be + "auto", "native", "emulate", "paravirt", or "smpsafe". + Other timers are always emulated. + </dd> + <dt><code>present</code></dt> + <dd> + The <code>present</code> attribute can be "yes" or "no" to + specify whether a particular timer is available to the guest. + </dd> + </dl> + </dd> </dl> <h3><a name="elementsDevices">Devices</a></h3> -- 1.7.2.1

2 2

[libvirt] [PATCH 0/4] Fix problem with capabilities XML generation
by Daniel P. Berrange 18 Aug '10

18 Aug '10

We have had sporadic reports of # virsh capabilities error: failed to get capabilities error: server closed connection: This normally means that libvirtd has crashed, closing the connection but in this case libvirtd has always remained running. It turns out that the capabilities XML was too large for the remote RPC message size. This caused XDR serialization to fail. This caused libvirtd to close the client connection immediately. The cause of the large XML was node handling an edge case in libnuma where it returns a CPU mask of all-1s to indicate a non-existant node. This series does many things: - Adds explicit warnings in places where XDR serialization fails, so we see an indication of problem in /var/log/messages - Try to send a real remote_error back to client, instead of closing its connection - Add logging of capabilities XML in libvirt.c so we can identify the too large doc in libvirtd - Add fix to cope with all-1s node mask

3 13

[libvirt] [PATCH] cygwin: build fix
by Stefan Berger 17 Aug '10

17 Aug '10

Fixing a problem in the build on cygwin due to missing #define's. Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com> --- src/remote/remote_protocol.c | 6 ++++++ src/remote/remote_protocol.h | 6 ++++++ src/remote/remote_protocol.x | 6 ++++++ 3 files changed, 18 insertions(+) Index: libvirt-acl/src/remote/remote_protocol.h =================================================================== --- libvirt-acl.orig/src/remote/remote_protocol.h +++ libvirt-acl/src/remote/remote_protocol.h @@ -24,6 +24,12 @@ extern "C" { #ifndef IXDR_GET_INT32 # define IXDR_GET_INT32 IXDR_GET_LONG #endif +#ifndef IXDR_PUT_U_INT32 +# define IXDR_PUT_U_INT32 IXDR_PUT_U_LONG +#endif +#ifndef IXDR_GET_U_INT32 +# define IXDR_GET_U_INT32 IXDR_GET_U_LONG +#endif #define REMOTE_MESSAGE_MAX 262144 #define REMOTE_MESSAGE_HEADER_MAX 24 #define REMOTE_MESSAGE_PAYLOAD_MAX 262120 Index: libvirt-acl/src/remote/remote_protocol.x =================================================================== --- libvirt-acl.orig/src/remote/remote_protocol.x +++ libvirt-acl/src/remote/remote_protocol.x @@ -51,6 +51,12 @@ %#ifndef IXDR_GET_INT32 %# define IXDR_GET_INT32 IXDR_GET_LONG %#endif +%#ifndef IXDR_PUT_U_INT32 +%# define IXDR_PUT_U_INT32 IXDR_PUT_U_LONG +%#endif +%#ifndef IXDR_GET_U_INT32 +%# define IXDR_GET_U_INT32 IXDR_GET_U_LONG +%#endif /*----- Data types. -----*/ Index: libvirt-acl/src/remote/remote_protocol.c =================================================================== --- libvirt-acl.orig/src/remote/remote_protocol.c +++ libvirt-acl/src/remote/remote_protocol.c @@ -16,6 +16,12 @@ #ifndef IXDR_GET_INT32 # define IXDR_GET_INT32 IXDR_GET_LONG #endif +#ifndef IXDR_PUT_U_INT32 +# define IXDR_PUT_U_INT32 IXDR_PUT_U_LONG +#endif +#ifndef IXDR_GET_U_INT32 +# define IXDR_GET_U_INT32 IXDR_GET_U_LONG +#endif bool_t xdr_remote_nonnull_string (XDR *xdrs, remote_nonnull_string *objp)

2 2

[libvirt] Potential gotcha in virsh migration with --persistent?
by Justin Clift 17 Aug '10

17 Aug '10

Hi all, Just noticed that when using the virsh --persistent option for migration, that the "autostart" attribute isn't mirrored across to the destination host. This seems like a "gotcha", in that a person migrating an autostarted guest to another host with --persistent will be expecting the migration to persist _all_ of the (important at least) guest settings. Without mirroring this attribute, the person doing the migration needs to manually follow up the migration by subsequently connecting to the destination host, then turning on autostart attribute for the domain there. Guessing it's not a design decision, and is just something we should fix? Regards and best wishes, Justin Clift

2 2

[libvirt] [PATCH 3/3] Remove wrong check for uml monitor response size
by Soren Hansen 17 Aug '10

17 Aug '10

The current check for the size of the response from the uml monitor is problematic for a couple of reasons: First of all, for me, the call to recvfrom returns 0, even though the buffer actually is populated with the response from the monitor. Second, it seems to me that (assuming recvfrom actually returned the number of bytes read) it should be compared against the size of the response header plus the actual data length rather than the max size of the datagram. At any rate, the only way I can get anything useful to happen here is to completely remove the check, so this patch does just that. Signed-off-by: Soren Hansen <soren(a)linux2go.dk> --- src/uml/uml_driver.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 04493ba..9cf669f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -737,10 +737,6 @@ static int umlMonitorCommand(const struct uml_driver *driver, virReportSystemError(errno, _("cannot read reply %s"), cmd); goto error; } - if (nbytes < sizeof res) { - virReportSystemError(0, _("incomplete reply %s"), cmd); - goto error; - } if (sizeof res.data < res.length) { virReportSystemError(0, _("invalid length in reply %s"), cmd); goto error; -- 1.7.0.4

4 12

[libvirt] [PATCH 0/3] qemu: Several PCI device assignment fixes
by Jiri Denemark 17 Aug '10

17 Aug '10

Jiri Denemark (3): qemu: Re-reserve all PCI addresses on libvirtd restart qemu: Release PCI slot when detaching disk and net devices qemu: Fix copy&paste error in warning message src/qemu/qemu_driver.c | 18 ++++++++++++++---- 1 files changed, 14 insertions(+), 4 deletions(-) -- 1.7.2

2 7

[libvirt] PATCH 4/4: AppArmor updates
by Jamie Strandboge 17 Aug '10

17 Aug '10

Attached is 0004-apparmor-fix-warn.patch -- Jamie Strandboge | http://www.canonical.com

3 2

[libvirt] [PATCH v3] nwfilter: extend nwfilter reload support
by Stefan Berger 17 Aug '10

17 Aug '10

This time with a proper title. v3: - Fixed an indentation problem - added bool parameter to function terminating the IP address learner threads to determine whether future threads may still run (needed in case of driver reload) or all must terminate (need in case of libvirtd termination) v2: - Fixes to the nwfilter driver reload function that also needs a valid virConnectPtr. In this patch I am extending and fixing the nwfilter module's reload support to stop all ongoing threads (for learning IP addresses of interfaces) and rebuild the filtering rules of all interfaces of all VMs when libvirt is started. Now libvirtd rebuilds the filters upon the SIGHUP signal and libvirtd restart. About the patch: The nwfilter functions require a virConnectPtr. Therefore I am opening a connection in qemudStartup, which later on needs to be closed outside where the driver lock is held since otherwise it ends up in a deadlock due to virConnectClose() trying to lock the driver as well. I have tested this now for a while with several machines running and needing the IP address learner thread(s). The rebuilding of the firewall rules seems to work fine following libvirtd restart or a SIGHUP. Also the termination of libvirtd worked fine. Signed-off-by: Stefan Berger<stefanb(a)us.ibm.com> --- src/nwfilter/nwfilter_driver.c | 21 +++++++++++--- src/nwfilter/nwfilter_learnipaddr.c | 16 ++++++++--- src/nwfilter/nwfilter_learnipaddr.h | 1 src/qemu/qemu_driver.c | 52 +++++++++++++++++++++++++++++++++--- 4 files changed, 77 insertions(+), 13 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_driver.c @@ -143,15 +143,26 @@ conf_init_err: */ static int nwfilterDriverReload(void) { + virConnectPtr conn; + if (!driverState) { return -1; } - nwfilterDriverLock(driverState); - virNWFilterPoolLoadAllConfigs(NULL, - &driverState->pools, - driverState->configDir); - nwfilterDriverUnlock(driverState); + conn = virConnectOpen("qemu:///system"); + + if (conn) { + /* shut down all threads -- qemud for example will restart them */ + virNWFilterLearnThreadsTerminate(true); + + nwfilterDriverLock(driverState); + virNWFilterPoolLoadAllConfigs(conn, + &driverState->pools, + driverState->configDir); + nwfilterDriverUnlock(driverState); + + virConnectClose(conn); + } return 0; } Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c @@ -857,6 +857,17 @@ virNWFilterLearnInit(void) { } +void +virNWFilterLearnThreadsTerminate(bool allowNewThreads) { + threadsTerminate = true; + + while (virHashSize(pendingLearnReq) != 0) + usleep((PKT_TIMEOUT_MS * 1000) / 3); + + if (allowNewThreads) + threadsTerminate = false; +} + /** * virNWFilterLearnShutdown * Shutdown of this layer @@ -864,10 +875,7 @@ virNWFilterLearnInit(void) { void virNWFilterLearnShutdown(void) { - threadsTerminate = true; - - while (virHashSize(pendingLearnReq) != 0) - usleep((PKT_TIMEOUT_MS * 1000) / 3); + virNWFilterLearnThreadsTerminate(false); virHashFree(pendingLearnReq, freeLearnReqEntry); pendingLearnReq = NULL; Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.h +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.h @@ -71,5 +71,6 @@ void virNWFilterUnlockIface(const char * int virNWFilterLearnInit(void); void virNWFilterLearnShutdown(void); +void virNWFilterLearnThreadsTerminate(bool allowNewThreads); #endif /* __NWFILTER_LEARNIPADDR_H */ Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -170,6 +170,9 @@ static int qemuDetectVcpuPIDs(struct qem static int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, virDomainDefPtr def); +static int qemudVMFiltersInstantiate(virConnectPtr conn, + virDomainDefPtr def); + static struct qemud_driver *qemu_driver = NULL; @@ -1423,6 +1426,10 @@ error: return ret; } +struct virReconnectDomainData { + virConnectPtr conn; + struct qemud_driver *driver; +}; /* * Open an existing VM's monitor, re-detect VCPU threads * and re-reserve the security labels in use @@ -1431,9 +1438,11 @@ static void qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) { virDomainObjPtr obj = payload; - struct qemud_driver *driver = opaque; + struct virReconnectDomainData *data = opaque; + struct qemud_driver *driver = data->driver; qemuDomainObjPrivatePtr priv; unsigned long long qemuCmdFlags; + virConnectPtr conn = data->conn; virDomainObjLock(obj); @@ -1467,6 +1476,9 @@ qemuReconnectDomain(void *payload, const obj) < 0) goto error; + if (qemudVMFiltersInstantiate(conn, obj->def)) + goto error; + if (obj->def->id >= driver->nextvmid) driver->nextvmid = obj->def->id + 1; @@ -1491,9 +1503,10 @@ error: * about. */ static void -qemuReconnectDomains(struct qemud_driver *driver) +qemuReconnectDomains(virConnectPtr conn, struct qemud_driver *driver) { - virHashForEach(driver->domains.objs, qemuReconnectDomain, driver); + struct virReconnectDomainData data = {conn, driver}; + virHashForEach(driver->domains.objs, qemuReconnectDomain, &data); } @@ -1691,6 +1704,7 @@ qemudStartup(int privileged) { char *base = NULL; char driverConf[PATH_MAX]; int rc; + virConnectPtr conn = NULL; if (VIR_ALLOC(qemu_driver) < 0) return -1; @@ -1912,7 +1926,11 @@ qemudStartup(int privileged) { 1, NULL, NULL) < 0) goto error; - qemuReconnectDomains(qemu_driver); + conn = virConnectOpen(qemu_driver->privileged ? + "qemu:///system" : + "qemu:///session"); + + qemuReconnectDomains(conn, qemu_driver); /* Then inactive persistent configs */ if (virDomainLoadAllConfigs(qemu_driver->caps, @@ -1930,6 +1948,8 @@ qemudStartup(int privileged) { qemudAutostartConfigs(qemu_driver); + if (conn) + virConnectClose(conn); return 0; @@ -1938,6 +1958,8 @@ out_of_memory: error: if (qemu_driver) qemuDriverUnlock(qemu_driver); + if (conn) + virConnectClose(conn); VIR_FREE(base); qemudShutdown(); return -1; @@ -12738,6 +12760,28 @@ qemudVMFilterRebuild(virConnectPtr conn return 0; } +static int +qemudVMFiltersInstantiate(virConnectPtr conn, + virDomainDefPtr def) +{ + int err = 0; + int i; + + if (!conn) + return 1; + + for (i = 0 ; i < def->nnets ; i++) { + virDomainNetDefPtr net = def->nets[i]; + if ((net->filter) && (net->ifname)) { + if (virDomainConfNWFilterInstantiate(conn, net)) { + err = 1; + break; + } + } + } + + return err; +} static virNWFilterCallbackDriver qemuCallbackDriver = { .name = "QEMU",

4 3

[libvirt] [RFC: PATCH 0/4] Update array allocation macros
by Eric Blake 17 Aug '10

17 Aug '10

I'm posing this series as an RFC for the moment. The basic premise is that I got bit trying to use VIR_REALLOC_N when working on the virCommand API, assuming that it zeroed out new memory because the documentation said so, only to hit a core dump. On looking closer, it became obvious to me that the VIR_REALLOC_N API is deficient - there's no way to know how much memory, if any, to zeroize without knowing the original size, so the new memory has to be uninitialized (explaining my crash), and the documentation is wrong. So, after some IRC discussions, two ideas emerged in addition to the obvious documentation update. One is the simpler VIR_EXPAND_N, where the array size always matches the array allocation, along with a VIR_SHRINK_N counterpart which can trim an array back down (and failure to trim need not be fatal). But this can scale quadratically if you end up with a large array, as the existing contents must be copied on every realloc. See patches 1 and 2 for an implementation and use. The other idea is VIR_RESIZE_N, which tracks allocation separately from usage, and has better scaling by doing geometric growth only when needed rather than on every array usage increase. But it takes more parameters (making it slightly harder to remember how to use correctly), along with possible modifications of the struct tracking an array (so it might not be possible to modify all uses of VIR_REALLOC_N to use this, if it would end up altering a public struct layout). See patches 3 and 4 for an implementation and use. Note that event.c is an example of code that already tracked allocation separately from usage, so the new VIR_RESIZE_N was very easy to use. On the other hand, libvirtd.c is an example of code where I had to add allocation tracking; notice the difference between patches 2/4 and 4/4 of the logic differences needed when using VIR_EXPAND_N vs. VIR_RESIZE_N. And notice that both patch 2 and patch 4 provide a net reduction in lines outside of memory.[ch]. Either way, I'm willing to audit all remaining uses of VIR_REALLOC_N (in fact, libvirtd.c has another use of VIR_REALLOC_N to allocate a buffer for getgrnam_r, where the uninitialized memory aspect was just fine and not worth converting). The only reason I'm posting this series as an RFC is to get a feel for which of the two styles I should prefer when doing my audit on the rest of the code base (rather than doing style A now only to find that style B would be preferred). Also, as written, the implementation forces all array count variables to be size_t if using VIR_EXPAND_N or VIR_RESIZE_N; I may come across a public API where I can't change the array count type, in which case I will have to rewrite the macros to be type-agnostic. Well, more like making the macro choose between a 4-byte or 8-byte implementation, based on sizeof(count), and adding a compiler verify() that count must be one of those two sizes, whether on 32-bit or 64-bit machines. Eric Blake (4): memory: make it safer to expand arrays daemon: use safer memory growth macros memory: make it easier to avoid quadratic scaling of arrays daemon: use more efficient memory growth macros daemon/event.c | 44 ++++++++++------------- daemon/libvirtd.c | 8 ++--- daemon/libvirtd.h | 11 +++--- docs/hacking.html.in | 61 ++++++++++++++++++++++++++------ src/util/memory.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++- src/util/memory.h | 77 +++++++++++++++++++++++++++++++++++++--- 6 files changed, 242 insertions(+), 53 deletions(-) -- 1.7.2.1

2 10