At 2010年11月23日 03:34, Eric Blake Write: This patch seems fine to me.

On 11/23/2010 04:45 AM, Stefan Berger wrote: Thanks. On reflection, I'm actually going to squash this in before pushing, since it is a more accurate description of what is going on. diff --git i/daemon/libvirtd.c w/daemon/libvirtd.c index aa2f6ec..66f1388 100644 --- i/daemon/libvirtd.c +++ w/daemon/libvirtd.c @@ -2362,7 +2362,8 @@ static void *qemudRunLoop(void *opaque) { server->clients + i + 1, sizeof (*server->clients) * (server->nclients - i)); - VIR_SHRINK_N(server->clients, server->nclients_max, 1); + VIR_SHRINK_N(server->clients, server->nclients_max, + server->nclients_max - server->nclients); goto reprocess; } } diff --git i/docs/hacking.html.in w/docs/hacking.html.in index 890692f..ac16f41 100644 --- i/docs/hacking.html.in +++ w/docs/hacking.html.in @@ -426,14 +426,15 @@ </pre> </li> - <li><p>To trim an array of domains to have one less element:</p> + <li><p>To trim an array of domains from its allocated size down + to the actual used size:</p> <pre> virDomainPtr domains; size_t ndomains = x; size_t ndomains_max = y; - VIR_SHRINK_N(domains, ndomains_max, 1); + VIR_SHRINK_N(domains, ndomains_max, ndomains_max - ndomains); </pre></li> <li><p>To free an array of domains:</p> -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

* src/util/memory.h (VIR_RESIZE_N): New macro. * src/util/memory.c (virResizeN): New function. * docs/hacking.html.in: Document it. * HACKING: Regenerate. * src/libvirt_private.syms: Export new helper. --- HACKING | 43 +++++++++++++++++++++++++++++++++++++---- docs/hacking.html.in | 47 ++++++++++++++++++++++++++++++++++++++++----- src/libvirt_private.syms | 1 + src/util/memory.c | 35 ++++++++++++++++++++++++++++++++++ src/util/memory.h | 26 +++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 11 deletions(-) diff --git a/HACKING b/HACKING index 2b19fe4..e54af05 100644 --- a/HACKING +++ b/HACKING @@ -308,7 +308,7 @@ routines, use the macros from memory.h. - To allocate an array of object pointers: virDomainPtr *domains; - int ndomains = 10; + size_t ndomains = 10; if (VIR_ALLOC_N(domains, ndomains) < 0) { virReportOOMError(); @@ -317,24 +317,57 @@ routines, use the macros from memory.h. -- To re-allocate the array of domains to be longer: +- To re-allocate the array of domains to be 1 element longer (however, note that +repeatedly expanding an array by 1 scales quadratically, so this is +recommended only for smaller arrays): + + virDomainPtr domains; + size_t ndomains = 0; - if (VIR_EXPAND_N(domains, ndomains, 10) < 0) { + if (VIR_EXPAND_N(domains, ndomains, 1) < 0) { virReportOOMError(); return NULL; } + domains[ndomains - 1] = domain; + + + +- To ensure an array has room to hold at least one more element (this approach +scales better, but requires tracking allocation separately from usage) + + virDomainPtr domains; + size_t ndomains = 0; + size_t ndomains_max = 0; + + if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) < 0) { + virReportOOMError(); + return NULL; + } + domains[ndomains++] = domain; - To trim an array of domains to have one less element: + virDomainPtr domains; + size_t ndomains = x; + size_t ndomains_max = y; + VIR_SHRINK_N(domains, ndomains, 1); -- To free the domain: +- To free an array of domains: - VIR_FREE(domain); + virDomainPtr domains; + size_t ndomains = x; + size_t ndomains_max = y; + size_t i; + + for (i = 0; i < ndomains; i++) + VIR_FREE(domains[i]); + VIR_FREE(domains) + ndomains_max = ndomains = 0; diff --git a/docs/hacking.html.in b/docs/hacking.html.in index ffe6aea..138a1ef 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -385,7 +385,7 @@ <li><p>To allocate an array of object pointers:</p> <pre> virDomainPtr *domains; - int ndomains = 10; + size_t ndomains = 10; if (VIR_ALLOC_N(domains, ndomains) &lt; 0) { virReportOOMError(); @@ -394,26 +394,61 @@ </pre> </li> - <li><p>To re-allocate the array of domains to be longer:</p> + <li><p>To re-allocate the array of domains to be 1 element + longer (however, note that repeatedly expanding an array by 1 + scales quadratically, so this is recommended only for smaller + arrays):</p> <pre> - if (VIR_EXPAND_N(domains, ndomains, 10) &lt; 0) { + virDomainPtr domains; + size_t ndomains = 0; + + if (VIR_EXPAND_N(domains, ndomains, 1) &lt; 0) { virReportOOMError(); return NULL; } + domains[ndomains - 1] = domain; +</pre></li> + + <li><p>To ensure an array has room to hold at least one more + element (this approach scales better, but requires tracking + allocation separately from usage)</p> + +<pre> + virDomainPtr domains; + size_t ndomains = 0; + size_t ndomains_max = 0; + + if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) &lt; 0) { + virReportOOMError(); + return NULL; + } + domains[ndomains++] = domain; </pre> </li> <li><p>To trim an array of domains to have one less element:</p> <pre> + virDomainPtr domains; + size_t ndomains = x; + size_t ndomains_max = y; + VIR_SHRINK_N(domains, ndomains, 1); </pre></li> - <li><p>To free the domain:</p> + <li><p>To free an array of domains:</p> <pre> - VIR_FREE(domain); + virDomainPtr domains; + size_t ndomains = x; + size_t ndomains_max = y; + size_t i; + + for (i = 0; i &lt; ndomains; i++) + VIR_FREE(domains[i]); + VIR_FREE(domains) + ndomains_max = ndomains = 0; </pre> - </li> + </li> </ul> <h2><a name="file_handling">File handling</a></h2> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 796559c..8bf1028 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -506,6 +506,7 @@ virAllocN; virExpandN; virFree; virReallocN; +virResizeN; virShrinkN; diff --git a/src/util/memory.c b/src/util/memory.c index 59685b3..d1237e3 100644 --- a/src/util/memory.c +++ b/src/util/memory.c @@ -198,6 +198,41 @@ int virExpandN(void *ptrptr, size_t size, size_t *countptr, size_t add) } /** + * virResizeN: + * @ptrptr: pointer to pointer for address of allocated memory + * @size: number of bytes per element + * @allocptr: pointer to number of elements allocated in array + * @count: number of elements currently used in array + * @add: minimum number of additional elements to support in array + * + * If 'count' + 'add' is larger than '*allocptr', then Resize the + * block of memory in 'ptrptr' to be an array of at least 'count' + + * 'add' elements, each 'size' bytes in length. Update 'ptrptr' and + * 'allocptr' with the details of the newly allocated memory. On + * failure, 'ptrptr' and 'allocptr' are not changed. Any newly + * allocated memory in 'ptrptr' is zero-filled. + * + * Returns -1 on failure to allocate, zero on success + */ +int virResizeN(void *ptrptr, size_t size, size_t *allocptr, size_t count, + size_t add) +{ + size_t delta; + + if (count + add < count) { + errno = ENOMEM; + return -1; + } + if (count + add <= *allocptr) + return 0; + + delta = count + add - *allocptr; + if (delta < *allocptr / 2) + delta = *allocptr / 2; + return virExpandN(ptrptr, size, allocptr, delta); +} + +/** * virShrinkN: * @ptrptr: pointer to pointer for address of allocated memory * @size: number of bytes per element diff --git a/src/util/memory.h b/src/util/memory.h index 98ac2b3..2b39393 100644 --- a/src/util/memory.h +++ b/src/util/memory.h @@ -54,6 +54,9 @@ int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); int virExpandN(void *ptrptr, size_t size, size_t *count, size_t add) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); +int virResizeN(void *ptrptr, size_t size, size_t *alloc, size_t count, + size_t desired) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); void virShrinkN(void *ptrptr, size_t size, size_t *count, size_t remove) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); int virAllocVar(void *ptrptr, @@ -117,6 +120,29 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); virExpandN(&(ptr), sizeof(*(ptr)), &(count), add) /** + * VIR_RESIZE_N: + * @ptr: pointer to hold address of allocated memory + * @alloc: variable tracking number of elements currently allocated + * @count: number of elements currently in use + * @add: minimum number of elements to additionally support + * + * Blindly using VIR_EXPAND_N(array, alloc, 1) in a loop scales + * quadratically, because every iteration must copy contents from + * all prior iterations. But amortized linear scaling can be achieved + * by tracking allocation size separately from the number of used + * elements, and growing geometrically only as needed. + * + * If 'count' + 'add' is larger than 'count', then geometrically reallocate + * the array of 'alloc' elements, each sizeof(*ptr) bytes long, and store + * the address of allocated memory in 'ptr' and the new size in 'alloc'. + * The new elements are filled with zero. + * + * Returns -1 on failure, 0 on success + */ +# define VIR_RESIZE_N(ptr, alloc, count, add) \ + virResizeN(&(ptr), sizeof(*(ptr)), &(alloc), count, add) + +/** * VIR_SHRINK_N: * @ptr: pointer to hold address of allocated memory * @count: variable tracking number of elements currently allocated -- 1.7.3.2

* daemon/libvirtd.h (qemud_server): Change types of members tracking array sizes, and add allocation trackers. * daemon/event.c (virEventLoop): Likewise. (virEventAddHandleImpl, virEventAddTimeoutImpl) (virEventCleanupTimeouts, virEventCleanupHandles): Use VIR_RESIZE_N instead of VIR_REALLOC_N. Tweak debug messages to match type changes. * daemon/libvirtd.c (qemudDispatchServer, qemudRunLoop): Likewise. --- daemon/event.c | 44 +++++++++++++++++++------------------------- daemon/libvirtd.c | 8 +++----- daemon/libvirtd.h | 11 ++++++----- 3 files changed, 28 insertions(+), 35 deletions(-) diff --git a/daemon/event.c b/daemon/event.c index 06f9aad..a983b35 100644 --- a/daemon/event.c +++ b/daemon/event.c @@ -73,11 +73,11 @@ struct virEventLoop { int running; virThread leader; int wakeupfd[2]; - int handlesCount; - int handlesAlloc; + size_t handlesCount; + size_t handlesAlloc; struct virEventHandle *handles; - int timeoutsCount; - int timeoutsAlloc; + size_t timeoutsCount; + size_t timeoutsAlloc; struct virEventTimeout *timeouts; }; @@ -103,14 +103,13 @@ int virEventAddHandleImpl(int fd, int events, EVENT_DEBUG("Add handle fd=%d events=%d cb=%p opaque=%p", fd, events, cb, opaque); virMutexLock(&eventLoop.lock); if (eventLoop.handlesCount == eventLoop.handlesAlloc) { - EVENT_DEBUG("Used %d handle slots, adding %d more", + EVENT_DEBUG("Used %zu handle slots, adding at least %d more", eventLoop.handlesAlloc, EVENT_ALLOC_EXTENT); - if (VIR_REALLOC_N(eventLoop.handles, - (eventLoop.handlesAlloc + EVENT_ALLOC_EXTENT)) < 0) { + if (VIR_RESIZE_N(eventLoop.handles, eventLoop.handlesAlloc, + eventLoop.handlesCount, EVENT_ALLOC_EXTENT) < 0) { virMutexUnlock(&eventLoop.lock); return -1; } - eventLoop.handlesAlloc += EVENT_ALLOC_EXTENT; } watch = nextWatch++; @@ -204,14 +203,13 @@ int virEventAddTimeoutImpl(int frequency, virMutexLock(&eventLoop.lock); if (eventLoop.timeoutsCount == eventLoop.timeoutsAlloc) { - EVENT_DEBUG("Used %d timeout slots, adding %d more", + EVENT_DEBUG("Used %zu timeout slots, adding at least %d more", eventLoop.timeoutsAlloc, EVENT_ALLOC_EXTENT); - if (VIR_REALLOC_N(eventLoop.timeouts, - (eventLoop.timeoutsAlloc + EVENT_ALLOC_EXTENT)) < 0) { + if (VIR_RESIZE_N(eventLoop.timeouts, eventLoop.timeoutsAlloc, + eventLoop.timeoutsCount, EVENT_ALLOC_EXTENT) < 0) { virMutexUnlock(&eventLoop.lock); return -1; } - eventLoop.timeoutsAlloc += EVENT_ALLOC_EXTENT; } eventLoop.timeouts[eventLoop.timeoutsCount].timer = nextTimer++; @@ -301,7 +299,7 @@ int virEventRemoveTimeoutImpl(int timer) { static int virEventCalculateTimeout(int *timeout) { unsigned long long then = 0; int i; - EVENT_DEBUG("Calculate expiry of %d timers", eventLoop.timeoutsCount); + EVENT_DEBUG("Calculate expiry of %zu timers", eventLoop.timeoutsCount); /* Figure out if we need a timeout */ for (i = 0 ; i < eventLoop.timeoutsCount ; i++) { if (eventLoop.timeouts[i].frequency < 0) @@ -482,7 +480,7 @@ static int virEventDispatchHandles(int nfds, struct pollfd *fds) { */ static int virEventCleanupTimeouts(void) { int i; - DEBUG("Cleanup %d", eventLoop.timeoutsCount); + DEBUG("Cleanup %zu", eventLoop.timeoutsCount); /* Remove deleted entries, shuffling down remaining * entries as needed to form contiguous series @@ -507,12 +505,10 @@ static int virEventCleanupTimeouts(void) { /* Release some memory if we've got a big chunk free */ if ((eventLoop.timeoutsAlloc - EVENT_ALLOC_EXTENT) > eventLoop.timeoutsCount) { - EVENT_DEBUG("Releasing %d out of %d timeout slots used, releasing %d", + EVENT_DEBUG("Releasing %zu out of %zu timeout slots used, releasing %d", eventLoop.timeoutsCount, eventLoop.timeoutsAlloc, EVENT_ALLOC_EXTENT); - if (VIR_REALLOC_N(eventLoop.timeouts, - (eventLoop.timeoutsAlloc - EVENT_ALLOC_EXTENT)) < 0) - return -1; - eventLoop.timeoutsAlloc -= EVENT_ALLOC_EXTENT; + VIR_SHRINK_N(eventLoop.timeouts, eventLoop.timeoutsAlloc, + EVENT_ALLOC_EXTENT); } return 0; } @@ -523,7 +519,7 @@ static int virEventCleanupTimeouts(void) { */ static int virEventCleanupHandles(void) { int i; - DEBUG("Cleanupo %d", eventLoop.handlesCount); + DEBUG("Cleanup %zu", eventLoop.handlesCount); /* Remove deleted entries, shuffling down remaining * entries as needed to form contiguous series @@ -547,12 +543,10 @@ static int virEventCleanupHandles(void) { /* Release some memory if we've got a big chunk free */ if ((eventLoop.handlesAlloc - EVENT_ALLOC_EXTENT) > eventLoop.handlesCount) { - EVENT_DEBUG("Releasing %d out of %d handles slots used, releasing %d", + EVENT_DEBUG("Releasing %zu out of %zu handles slots used, releasing %d", eventLoop.handlesCount, eventLoop.handlesAlloc, EVENT_ALLOC_EXTENT); - if (VIR_REALLOC_N(eventLoop.handles, - (eventLoop.handlesAlloc - EVENT_ALLOC_EXTENT)) < 0) - return -1; - eventLoop.handlesAlloc -= EVENT_ALLOC_EXTENT; + VIR_SHRINK_N(eventLoop.handles, eventLoop.handlesAlloc, + EVENT_ALLOC_EXTENT); } return 0; } diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 1673e91..e544c48 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1333,7 +1333,8 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket goto error; } - if (VIR_REALLOC_N(server->clients, server->nclients+1) < 0) { + if (VIR_RESIZE_N(server->clients, server->nclients_max, + server->nclients, 1) < 0) { VIR_ERROR0(_("Out of memory allocating clients")); goto error; } @@ -2361,10 +2362,7 @@ static void *qemudRunLoop(void *opaque) { server->clients + i + 1, sizeof (*server->clients) * (server->nclients - i)); - if (VIR_REALLOC_N(server->clients, - server->nclients) < 0) { - ; /* ignore */ - } + VIR_SHRINK_N(server->clients, server->nclients, 0); goto reprocess; } } diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h index 785ac07..af20e56 100644 --- a/daemon/libvirtd.h +++ b/daemon/libvirtd.h @@ -1,7 +1,7 @@ /* * libvirtd.h: daemon data structure definitions * - * Copyright (C) 2006-2009 Red Hat, Inc. + * Copyright (C) 2006-2010 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -283,12 +283,13 @@ struct qemud_server { int privileged; - int nworkers; - int nactiveworkers; + size_t nworkers; + size_t nactiveworkers; struct qemud_worker *workers; - int nsockets; + size_t nsockets; struct qemud_socket *sockets; - int nclients; + size_t nclients; + size_t nclients_max; struct qemud_client **clients; int sigread; -- 1.7.3.2

* src/conf/capabilities.h (_virCaps, _virCapsHost, _virCapsGuest) (_virCapsGuestArch): Add additional fields. * src/conf/cpu_conf.h (_virCPUDef): Likewise. * src/conf/capabilities.c (virCapabilitiesFormatXML): Reflect updated type. (virCapabilitiesAddGuest, virCapabilitiesAddHostFeature) (virCapabilitiesAddHostMigrateTransport) (virCapabilitiesAddHostNUMACell, virCapabilitiesAddGuestFeature) (virCapabilitiesAddGuestDomain): Use new array APIs. * src/conf/cpu_conf.c (virCPUDefAddFeature, virCPUDefCopy) (virCPUDefParseXML): Likewise. * tests/testutilsqemu.c (testQemuCapsInit): Adjust test. --- src/conf/capabilities.c | 37 +++++++++++++++++-------------------- src/conf/capabilities.h | 20 +++++++++++++------- src/conf/cpu_conf.c | 7 +++++-- src/conf/cpu_conf.h | 5 +++-- tests/testutilsqemu.c | 1 + 5 files changed, 39 insertions(+), 31 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 4478d85..99d5a56 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1,7 +1,7 @@ /* * capabilities.c: hypervisor capabilities * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2008, 2010 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -190,8 +190,8 @@ int virCapabilitiesAddHostFeature(virCapsPtr caps, const char *name) { - if (VIR_REALLOC_N(caps->host.features, - caps->host.nfeatures + 1) < 0) + if (VIR_RESIZE_N(caps->host.features, caps->host.nfeatures_max, + caps->host.nfeatures, 1) < 0) return -1; if ((caps->host.features[caps->host.nfeatures] = strdup(name)) == NULL) @@ -213,8 +213,8 @@ int virCapabilitiesAddHostMigrateTransport(virCapsPtr caps, const char *name) { - if (VIR_REALLOC_N(caps->host.migrateTrans, - caps->host.nmigrateTrans + 1) < 0) + if (VIR_RESIZE_N(caps->host.migrateTrans, caps->host.nmigrateTrans_max, + caps->host.nmigrateTrans, 1) < 0) return -1; if ((caps->host.migrateTrans[caps->host.nmigrateTrans] = strdup(name)) == NULL) @@ -243,8 +243,8 @@ virCapabilitiesAddHostNUMACell(virCapsPtr caps, { virCapsHostNUMACellPtr cell; - if (VIR_REALLOC_N(caps->host.numaCell, - caps->host.nnumaCell + 1) < 0) + if (VIR_RESIZE_N(caps->host.numaCell, caps->host.nnumaCell_max, + caps->host.nnumaCell, 1) < 0) return -1; if (VIR_ALLOC(cell) < 0) @@ -261,8 +261,7 @@ virCapabilitiesAddHostNUMACell(virCapsPtr caps, cell->ncpus = ncpus; cell->num = num; - caps->host.numaCell[caps->host.nnumaCell] = cell; - caps->host.nnumaCell++; + caps->host.numaCell[caps->host.nnumaCell++] = cell; return 0; } @@ -380,11 +379,10 @@ virCapabilitiesAddGuest(virCapsPtr caps, (guest->arch.defaultInfo.loader = strdup(loader)) == NULL) goto no_memory; - if (VIR_REALLOC_N(caps->guests, - caps->nguests + 1) < 0) + if (VIR_RESIZE_N(caps->guests, caps->nguests_max, + caps->nguests, 1) < 0) goto no_memory; - caps->guests[caps->nguests] = guest; - caps->nguests++; + caps->guests[caps->nguests++] = guest; if (nmachines) { guest->arch.defaultInfo.nmachines = nmachines; @@ -434,8 +432,8 @@ virCapabilitiesAddGuestDomain(virCapsGuestPtr guest, (dom->info.loader = strdup(loader)) == NULL) goto no_memory; - if (VIR_REALLOC_N(guest->arch.domains, - guest->arch.ndomains + 1) < 0) + if (VIR_RESIZE_N(guest->arch.domains, guest->arch.ndomains_max, + guest->arch.ndomains, 1) < 0) goto no_memory; guest->arch.domains[guest->arch.ndomains] = dom; guest->arch.ndomains++; @@ -478,11 +476,10 @@ virCapabilitiesAddGuestFeature(virCapsGuestPtr guest, feature->defaultOn = defaultOn; feature->toggle = toggle; - if (VIR_REALLOC_N(guest->features, - guest->nfeatures + 1) < 0) + if (VIR_RESIZE_N(guest->features, guest->nfeatures_max, + guest->nfeatures, 1) < 0) goto no_memory; - guest->features[guest->nfeatures] = feature; - guest->nfeatures++; + guest->features[guest->nfeatures++] = feature; return feature; @@ -706,7 +703,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) if (caps->host.nnumaCell) { virBufferAddLit(&xml, " <topology>\n"); - virBufferVSprintf(&xml, " <cells num='%d'>\n", + virBufferVSprintf(&xml, " <cells num='%zu'>\n", caps->host.nnumaCell); for (i = 0 ; i < caps->host.nnumaCell ; i++) { virBufferVSprintf(&xml, " <cell id='%d'>\n", diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index f41be1c..759265d 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -1,7 +1,7 @@ /* * capabilities.h: hypervisor capabilities * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2008, 2010 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -71,7 +71,8 @@ struct _virCapsGuestArch { char *name; int wordsize; virCapsGuestDomainInfo defaultInfo; - int ndomains; + size_t ndomains; + size_t ndomains_max; virCapsGuestDomainPtr *domains; }; @@ -80,7 +81,8 @@ typedef virCapsGuest *virCapsGuestPtr; struct _virCapsGuest { char *ostype; virCapsGuestArch arch; - int nfeatures; + size_t nfeatures; + size_t nfeatures_max; virCapsGuestFeaturePtr *features; }; @@ -102,13 +104,16 @@ typedef struct _virCapsHost virCapsHost; typedef virCapsHost *virCapsHostPtr; struct _virCapsHost { char *arch; - int nfeatures; + size_t nfeatures; + size_t nfeatures_max; char **features; int offlineMigrate; int liveMigrate; - int nmigrateTrans; + size_t nmigrateTrans; + size_t nmigrateTrans_max; char **migrateTrans; - int nnumaCell; + size_t nnumaCell; + size_t nnumaCell_max; virCapsHostNUMACellPtr *numaCell; virCapsHostSecModel secModel; virCPUDefPtr cpu; @@ -134,7 +139,8 @@ typedef struct _virCaps virCaps; typedef virCaps* virCapsPtr; struct _virCaps { virCapsHost host; - int nguests; + size_t nguests; + size_t nguests_max; virCapsGuestPtr *guests; unsigned char macPrefix[VIR_MAC_PREFIX_BUFLEN]; unsigned int emulatorRequired : 1; diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index f8d006d..7f03eaa 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -83,6 +83,7 @@ virCPUDefCopy(const virCPUDefPtr cpu) || (cpu->vendor && !(copy->vendor = strdup(cpu->vendor))) || VIR_ALLOC_N(copy->features, cpu->nfeatures) < 0) goto no_memory; + copy->nfeatures_max = cpu->nfeatures; copy->type = cpu->type; copy->match = cpu->match; @@ -234,7 +235,8 @@ virCPUDefParseXML(const xmlNodePtr node, goto error; } - if (VIR_ALLOC_N(def->features, n) < 0) + if (VIR_RESIZE_N(def->features, def->nfeatures_max, + def->nfeatures, n) < 0) goto no_memory; def->nfeatures = n; } @@ -425,7 +427,8 @@ virCPUDefAddFeature(virCPUDefPtr def, } } - if (VIR_REALLOC_N(def->features, def->nfeatures + 1) < 0) + if (VIR_RESIZE_N(def->features, def->nfeatures_max, + def->nfeatures, 1) < 0) goto no_memory; if (def->type == VIR_CPU_TYPE_HOST) diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 6151683..055887c 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -1,7 +1,7 @@ /* * cpu_conf.h: CPU XML handling * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009, 2010 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -74,7 +74,8 @@ struct _virCPUDef { unsigned int sockets; unsigned int cores; unsigned int threads; - unsigned int nfeatures; + size_t nfeatures; + size_t nfeatures_max; virCPUFeatureDefPtr features; }; diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index dd33b62..72fc8aa 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -89,6 +89,7 @@ virCapsPtr testQemuCapsInit(void) { 2, /* cores */ 1, /* threads */ ARRAY_CARDINALITY(host_cpu_features), /* nfeatures */ + ARRAY_CARDINALITY(host_cpu_features), /* nfeatures_max */ host_cpu_features /* features */ }; -- 1.7.3.2

From: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/util/util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 2d6485f..f2fe58a 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -463,7 +463,7 @@ __virExec(const char *const*argv, int childerr = -1; int tmpfd; - if ((null = open("/dev/null", O_RDONLY)) < 0) { + if ((null = open("/dev/null", O_RDWR)) < 0) { virReportSystemError(errno, _("cannot open %s"), "/dev/null"); -- 1.7.3.2

Using 'int ret = strcmp(a, b)' in a qsort function is a valid use of str[n]cmp that should _not_ be turned to STREQ, but it was falling foul of our specific syntax-check. Meanwhile, gnulib's maint.mk already has a tighter bound for strcmp, so we can copy that regex and just check for strncmp, which results in fewer false positives that require exceptions. * cfg.mk (sc_prohibit_strcmp_and_strncmp): Rename... (sc_prohibit_strncmp): ...to this, and tighten, to mirror maint.mk's sc_prohibit_strcmp's better regex. * Makefile.am (syntax_check_exceptions): Update exception rule. * .x-sc_prohibit_strcmp_and_strncmp: Rename... * .x-sc_prohibit_strncmp: ...and trim. --- .x-sc_prohibit_strcmp_and_strncmp | 9 --------- .x-sc_prohibit_strncmp | 1 + Makefile.am | 2 +- cfg.mk | 14 ++++++++------ 4 files changed, 10 insertions(+), 16 deletions(-) delete mode 100644 .x-sc_prohibit_strcmp_and_strncmp create mode 100644 .x-sc_prohibit_strncmp diff --git a/.x-sc_prohibit_strcmp_and_strncmp b/.x-sc_prohibit_strcmp_and_strncmp deleted file mode 100644 index 77c3ee6..0000000 --- a/.x-sc_prohibit_strcmp_and_strncmp +++ /dev/null @@ -1,9 +0,0 @@ -^gnulib/ -^ChangeLog-old$ -^docs/ -^examples/domain-events/events-c/event-test\.c$ -^src/internal\.h$ -^src/lxc/lxc_container\.c$ -^src/node_device/node_device_devkit\.c$ -^src/node_device/node_device_hal\.c$ -^src/storage/parthelper\.c$ diff --git a/.x-sc_prohibit_strncmp b/.x-sc_prohibit_strncmp new file mode 100644 index 0000000..8be2055 --- /dev/null +++ b/.x-sc_prohibit_strncmp @@ -0,0 +1 @@ +^src/internal\.h$ diff --git a/Makefile.am b/Makefile.am index d34313a..efdc204 100644 --- a/Makefile.am +++ b/Makefile.am @@ -33,7 +33,7 @@ syntax_check_exceptions = \ .x-sc_prohibit_readlink \ .x-sc_prohibit_sprintf \ .x-sc_prohibit_strcmp \ - .x-sc_prohibit_strcmp_and_strncmp \ + .x-sc_prohibit_strncmp \ .x-sc_prohibit_strncpy \ .x-sc_prohibit_test_minus_ao \ .x-sc_prohibit_VIR_ERR_NO_MEMORY \ diff --git a/cfg.mk b/cfg.mk index 19a4622..1863bf1 100644 --- a/cfg.mk +++ b/cfg.mk @@ -231,12 +231,14 @@ sc_avoid_write: halt='consider using safewrite instead of write' \ $(_sc_search_regexp) -# Use STREQ rather than comparing strcmp == 0, or != 0. -# Similarly, use STREQLEN or STRPREFIX rather than strncmp. -sc_prohibit_strcmp_and_strncmp: - @prohibit='strn?cmp *\(' \ - halt='use STREQ() in place of the above uses of str[n]cmp' \ - $(_sc_search_regexp) +# Similar to the gnulib maint.mk rule for sc_prohibit_strcmp +# Use STREQLEN or STRPREFIX rather than comparing strncmp == 0, or != 0. +sc_prohibit_strncmp: + @grep -nE '! *str''ncmp *\(|\<str''ncmp *\([^)]+\) *==' \ + $$($(VC_LIST_EXCEPT)) \ + | grep -vE ':# *define STREQ\(' && \ + { echo '$(ME): use STREQLEN or STRPREFIX instead of str''ncmp' \ + 1>&2; exit 1; } || : # Use virAsprintf rather than as'printf since *strp is undefined on error. sc_prohibit_asprintf: -- 1.7.3.2

On Wed, Nov 17, 2010 at 09:28:59PM -0700, Eric Blake wrote: You might want to re-check this one - when I re-tested with macvtap enabled, it caused a build failure. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

From: Daniel P. Berrange <berrange(a)redhat.com&gt; This introduces a new set of APIs in src/util/command.h to use for invoking commands. This is intended to replace all current usage of virRun and virExec variants, with a more flexible and less error prone API. * src/util/command.c: New file. * src/util/command.h: New header. * src/Makefile.am (UTIL_SOURCES): Build it. * src/libvirt_private.syms: Export symbols internally. * tests/commandtest.c: New test. * tests/Makefile.am (check_PROGRAMS): Run it. * tests/commandhelper.c: Auxiliary program. * tests/commanddata/test2.log - test15.log: New expected outputs. * cfg.mk (useless_free_options): Add virCommandFree. * po/POTFILES.in: New translation. * .x-sc_avoid_write: Add exemption. * tests/.gitignore: Ignore new built file. --- .x-sc_avoid_write | 1 + cfg.mk | 1 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 29 ++ src/util/command.c | 960 ++++++++++++++++++++++++++++++++++++++++++ src/util/command.h | 213 ++++++++++ tests/.gitignore | 4 + tests/Makefile.am | 18 +- tests/commanddata/test10.log | 14 + tests/commanddata/test11.log | 14 + tests/commanddata/test12.log | 12 + tests/commanddata/test13.log | 12 + tests/commanddata/test14.log | 12 + tests/commanddata/test15.log | 12 + tests/commanddata/test2.log | 12 + tests/commanddata/test3.log | 14 + tests/commanddata/test4.log | 12 + tests/commanddata/test5.log | 10 + tests/commanddata/test6.log | 6 + tests/commanddata/test7.log | 11 + tests/commanddata/test8.log | 7 + tests/commanddata/test9.log | 18 + tests/commandhelper.c | 136 ++++++ tests/commandtest.c | 572 +++++++++++++++++++++++++ 25 files changed, 2101 insertions(+), 1 deletions(-) create mode 100644 src/util/command.c create mode 100644 src/util/command.h create mode 100644 tests/commanddata/test10.log create mode 100644 tests/commanddata/test11.log create mode 100644 tests/commanddata/test12.log create mode 100644 tests/commanddata/test13.log create mode 100644 tests/commanddata/test14.log create mode 100644 tests/commanddata/test15.log create mode 100644 tests/commanddata/test2.log create mode 100644 tests/commanddata/test3.log create mode 100644 tests/commanddata/test4.log create mode 100644 tests/commanddata/test5.log create mode 100644 tests/commanddata/test6.log create mode 100644 tests/commanddata/test7.log create mode 100644 tests/commanddata/test8.log create mode 100644 tests/commanddata/test9.log create mode 100644 tests/commandhelper.c create mode 100644 tests/commandtest.c diff --git a/.x-sc_avoid_write b/.x-sc_avoid_write index 232504f..f6fc1b2 100644 --- a/.x-sc_avoid_write +++ b/.x-sc_avoid_write @@ -1,6 +1,7 @@ ^src/libvirt\.c$ ^src/fdstream\.c$ ^src/qemu/qemu_monitor\.c$ +^src/util/command\.c$ ^src/util/util\.c$ ^src/xen/xend_internal\.c$ ^daemon/libvirtd.c$ diff --git a/cfg.mk b/cfg.mk index 1863bf1..3b29ca1 100644 --- a/cfg.mk +++ b/cfg.mk @@ -77,6 +77,7 @@ useless_free_options = \ --name=virCapabilitiesFreeHostNUMACell \ --name=virCapabilitiesFreeMachines \ --name=virCgroupFree \ + --name=virCommandFree \ --name=virConfFreeList \ --name=virConfFreeValue \ --name=virDomainChrDefFree \ diff --git a/po/POTFILES.in b/po/POTFILES.in index 2820ac1..e7be0d3 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -78,6 +78,7 @@ src/uml/uml_driver.c src/util/authhelper.c src/util/bridge.c src/util/cgroup.c +src/util/command.c src/util/conf.c src/util/dnsmasq.c src/util/hooks.c diff --git a/src/Makefile.am b/src/Makefile.am index a9a1986..0923d60 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -48,6 +48,7 @@ UTIL_SOURCES = \ util/bitmap.c util/bitmap.h \ util/bridge.c util/bridge.h \ util/buf.c util/buf.h \ + util/command.c util/command.h \ util/conf.c util/conf.h \ util/cgroup.c util/cgroup.h \ util/event.c util/event.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8bf1028..7a7ede6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -82,6 +82,35 @@ virCgroupSetMemorySoftLimit; virCgroupSetSwapHardLimit; +# command.h +virCommandAddArg; +virCommandAddArgList; +virCommandAddArgPair; +virCommandAddArgSet; +virCommandAddEnvPair; +virCommandAddEnvPass; +virCommandAddEnvPassCommon; +virCommandAddEnvString; +virCommandClearCaps; +virCommandDaemonize; +virCommandFree; +virCommandNew; +virCommandNewArgs; +virCommandPreserveFD; +virCommandRun; +virCommandRunAsync; +virCommandSetErrorBuffer; +virCommandSetErrorFD; +virCommandSetInputBuffer; +virCommandSetInputFD; +virCommandSetOutputBuffer; +virCommandSetOutputFD; +virCommandSetPidFile; +virCommandSetPreExecHook; +virCommandSetWorkingDirectory; +virCommandWait; + + # conf.h virConfFree; virConfFreeValue; diff --git a/src/util/command.c b/src/util/command.c new file mode 100644 index 0000000..71db2a1 --- /dev/null +++ b/src/util/command.c @@ -0,0 +1,960 @@ +/* + * command.c: Child command execution + * + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#include <config.h> + +#include <poll.h> +#include <stdarg.h> +#include <stdbool.h> +#include <stdlib.h> +#include <sys/wait.h> + +#include "command.h" +#include "memory.h" +#include "virterror_internal.h" +#include "util.h" +#include "logging.h" +#include "files.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +#define virCommandError(code, ...) \ + virReportErrorHelper(NULL, VIR_FROM_NONE, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + +struct _virCommand { + int has_error; /* ENOMEM on allocation failure, -1 for anything else. */ + + char **args; + size_t nargs; + size_t maxargs; + + char **env; + size_t nenv; + size_t maxenv; + + char *pwd; + + /* XXX Use int[] if we ever need to support more than FD_SETSIZE fd's. */ + fd_set preserve; + unsigned int flags; + + char *inbuf; + char **outbuf; + char **errbuf; + + int infd; + int inpipe; + int outfd; + int errfd; + int *outfdptr; + int *errfdptr; + + virExecHook hook; + void *opaque; + + pid_t pid; + char *pidfile; +}; + +/* + * Create a new command for named binary + */ +virCommandPtr +virCommandNew(const char *binary) +{ + const char *const args[] = { binary, NULL }; + + return virCommandNewArgs(args); +} + +/* + * Create a new command with a NULL terminated + * set of args, taking binary from argv[0] + */ +virCommandPtr +virCommandNewArgs(const char *const*args) +{ + virCommandPtr cmd; + + if (VIR_ALLOC(cmd) < 0) + return NULL; + + FD_ZERO(&cmd->preserve); + cmd->infd = cmd->outfd = cmd->errfd = -1; + cmd->inpipe = -1; + cmd->pid = -1; + + virCommandAddArgSet(cmd, args); + + if (cmd->has_error) { + virCommandFree(cmd); + return NULL; + } + + return cmd; +} + + +/* + * Preserve the specified file descriptor in the child, instead of + * closing it. FD must not be one of the three standard streams. + */ +void +virCommandPreserveFD(virCommandPtr cmd, int fd) +{ + if (!cmd || cmd->has_error) + return; + + if (fd <= STDERR_FILENO || FD_SETSIZE <= fd) { + cmd->has_error = -1; + VIR_DEBUG("cannot preserve %d", fd); + return; + } + + FD_SET(fd, &cmd->preserve); +} + +/* + * Save the child PID in a pidfile + */ +void +virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) +{ + if (!cmd || cmd->has_error) + return; + + VIR_FREE(cmd->pidfile); + if (!(cmd->pidfile = strdup(pidfile))) { + cmd->has_error = ENOMEM; + } +} + + +/* + * Remove all capabilities from the child + */ +void +virCommandClearCaps(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error) + return; + + cmd->flags |= VIR_EXEC_CLEAR_CAPS; +} + +#if 0 /* XXX Enable if we have a need for capability management. */ + +/* + * Re-allow a specific capability + */ +void +virCommandAllowCap(virCommandPtr cmd, + int capability ATTRIBUTE_UNUSED) +{ + if (!cmd || cmd->has_error) + return; + + /* XXX ? */ +} + +#endif /* 0 */ + + +/* + * Daemonize the child process + */ +void +virCommandDaemonize(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error) + return; + + cmd->flags |= VIR_EXEC_DAEMON; +} + +/* + * Add an environment variable to the child + * using separate name & value strings + */ +void +virCommandAddEnvPair(virCommandPtr cmd, const char *name, const char *value) +{ + char *env; + + if (!cmd || cmd->has_error) + return; + + if (virAsprintf(&env, "%s=%s", name, value ? value : "") < 0) { + cmd->has_error = ENOMEM; + return; + } + + /* env plus trailing NULL */ + if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { + VIR_FREE(env); + cmd->has_error = ENOMEM; + return; + } + + cmd->env[cmd->nenv++] = env; +} + + +/* + * Add an environment variable to the child + * using a preformatted env string FOO=BAR + */ +void +virCommandAddEnvString(virCommandPtr cmd, const char *str) +{ + if (!cmd || cmd->has_error) + return; + + char *env; + + if (!cmd || cmd->has_error) + return; + + if (!(env = strdup(str))) { + cmd->has_error = ENOMEM; + return; + } + + /* env plus trailing NULL */ + if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { + VIR_FREE(env); + cmd->has_error = ENOMEM; + return; + } + + cmd->env[cmd->nenv++] = env; +} + + +/* + * Pass an environment variable to the child + * using current process' value + */ +void +virCommandAddEnvPass(virCommandPtr cmd, const char *name) +{ + char *value; + if (!cmd || cmd->has_error) + return; + + value = getenv(name); + if (value) + virCommandAddEnvPair(cmd, name, value); +} + + +/* + * Set LC_ALL to C, and propagate other essential environment + * variables from the parent process. + */ +void +virCommandAddEnvPassCommon(virCommandPtr cmd) +{ + /* Attempt to Pre-allocate; allocation failure will be detected + * later during virCommandAdd*. */ + ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9)); + + virCommandAddEnvPair(cmd, "LC_ALL", "C"); + + virCommandAddEnvPass(cmd, "LD_PRELOAD"); + virCommandAddEnvPass(cmd, "LD_LIBRARY_PATH"); + virCommandAddEnvPass(cmd, "PATH"); + virCommandAddEnvPass(cmd, "HOME"); + virCommandAddEnvPass(cmd, "USER"); + virCommandAddEnvPass(cmd, "LOGNAME"); + virCommandAddEnvPass(cmd, "TMPDIR"); +} + +/* + * Add a command line argument to the child + */ +void +virCommandAddArg(virCommandPtr cmd, const char *val) +{ + char *arg; + + if (!cmd || cmd->has_error) + return; + + if (!(arg = strdup(val))) { + cmd->has_error = ENOMEM; + return; + } + + /* Arg plus trailing NULL. */ + if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { + VIR_FREE(arg); + cmd->has_error = ENOMEM; + return; + } + + cmd->args[cmd->nargs++] = arg; +} + + +/* + * Add "NAME=VAL" as a single command line argument to the child + */ +void +virCommandAddArgPair(virCommandPtr cmd, const char *name, const char *val) +{ + char *arg; + + if (!cmd || cmd->has_error) + return; + + if (virAsprintf(&arg, "%s=%s", name, val) < 0) { + cmd->has_error = ENOMEM; + return; + } + + /* Arg plus trailing NULL. */ + if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { + VIR_FREE(arg); + cmd->has_error = ENOMEM; + return; + } + + cmd->args[cmd->nargs++] = arg; +} + +/* + * Add a NULL terminated list of args + */ +void +virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) +{ + int narg = 0; + + if (!cmd || cmd->has_error) + return; + + while (vals[narg] != NULL) + narg++; + + /* narg plus trailing NULL. */ + if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, narg + 1) < 0) { + cmd->has_error = ENOMEM; + return; + } + + narg = 0; + while (vals[narg] != NULL) { + char *arg = strdup(vals[narg++]); + if (!arg) { + cmd->has_error = ENOMEM; + return; + } + cmd->args[cmd->nargs++] = arg; + } +} + +/* + * Add a NULL terminated list of args + */ +void +virCommandAddArgList(virCommandPtr cmd, ...) +{ + va_list list; + int narg = 0; + + if (!cmd || cmd->has_error) + return; + + va_start(list, cmd); + while (va_arg(list, const char *) != NULL) + narg++; + va_end(list); + + /* narg plus trailing NULL. */ + if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, narg + 1) < 0) { + cmd->has_error = ENOMEM; + return; + } + + va_start(list, cmd); + while (1) { + char *arg = va_arg(list, char *); + if (!arg) + break; + arg = strdup(arg); + if (!arg) { + cmd->has_error = ENOMEM; + va_end(list); + return; + } + cmd->args[cmd->nargs++] = arg; + } + va_end(list); +} + +/* + * Set the working directory of a non-daemon child process, rather + * than the parent's working directory. Daemons automatically get / + * without using this call. + */ +void +virCommandSetWorkingDirectory(virCommandPtr cmd, const char *pwd) +{ + if (!cmd || cmd->has_error) + return; + + if (cmd->pwd) { + cmd->has_error = -1; + VIR_DEBUG0("cannot set directory twice"); + } else { + cmd->pwd = strdup(pwd); + if (!cmd->pwd) + cmd->has_error = ENOMEM; + } +} + + +/* + * Feed the child's stdin from a string buffer + */ +void +virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf) +{ + if (!cmd || cmd->has_error) + return; + + if (cmd->infd != -1 || cmd->inbuf) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify input twice"); + return; + } + + cmd->inbuf = strdup(inbuf); + if (!cmd->inbuf) + cmd->has_error = ENOMEM; +} + + +/* + * Capture the child's stdout to a string buffer + */ +void +virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf) +{ + if (!cmd || cmd->has_error) + return; + + if (cmd->outfd != -1) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify output twice"); + return; + } + + cmd->outbuf = outbuf; + cmd->outfdptr = &cmd->outfd; +} + + +/* + * Capture the child's stderr to a string buffer + */ +void +virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf) +{ + if (!cmd || cmd->has_error) + return; + + if (cmd->errfd != -1) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify stderr twice"); + return; + } + + cmd->errbuf = errbuf; + cmd->errfdptr = &cmd->errfd; +} + + +/* + * Attach a file descriptor to the child's stdin + */ +void +virCommandSetInputFD(virCommandPtr cmd, int infd) +{ + if (!cmd || cmd->has_error) + return; + + if (infd < 0 || cmd->inbuf) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify input twice"); + return; + } + + cmd->infd = infd; +} + + +/* + * Attach a file descriptor to the child's stdout + */ +void +virCommandSetOutputFD(virCommandPtr cmd, int *outfd) +{ + if (!cmd || cmd->has_error) + return; + + if (!outfd || cmd->outfd != -1) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify output twice"); + return; + } + + cmd->outfdptr = outfd; +} + + +/* + * Attach a file descriptor to the child's stderr + */ +void +virCommandSetErrorFD(virCommandPtr cmd, int *errfd) +{ + if (!cmd || cmd->has_error) + return; + + if (!errfd || cmd->errfd != -1) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify stderr twice"); + return; + } + + cmd->errfdptr = errfd; +} + + +/* + * Run HOOK(OPAQUE) in the child as the last thing before changing + * directories, dropping capabilities, and executing the new process. + * Force the child to fail if HOOK does not return zero. + */ +void +virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque) +{ + if (!cmd || cmd->has_error) + return; + + cmd->hook = hook; + cmd->opaque = opaque; +} + + +/* + * Manage input and output to the child process. + */ +static int +virCommandProcessIO(virCommandPtr cmd) +{ + int infd = -1, outfd = -1, errfd = -1; + size_t inlen = 0, outlen = 0, errlen = 0; + size_t inoff = 0; + + /* With an input buffer, feed data to child + * via pipe */ + if (cmd->inbuf) { + inlen = strlen(cmd->inbuf); + infd = cmd->inpipe; + } + + /* With out/err buffer, the outfd/errfd + * have been filled with an FD for us */ + if (cmd->outbuf) + outfd = cmd->outfd; + if (cmd->errbuf) + errfd = cmd->errfd; + + for (;;) { + int i; + struct pollfd fds[3]; + int nfds = 0; + + if (infd != -1) { + fds[nfds].fd = infd; + fds[nfds].events = POLLOUT; + nfds++; + } + if (outfd != -1) { + fds[nfds].fd = outfd; + fds[nfds].events = POLLIN; + nfds++; + } + if (errfd != -1) { + fds[nfds].fd = errfd; + fds[nfds].events = POLLIN; + nfds++; + } + + if (nfds == 0) + break; + + if (poll(fds,nfds, -1) < 0) { + if ((errno == EAGAIN) || (errno == EINTR)) + continue; + virReportSystemError(errno, "%s", + _("unable to poll on child")); + return -1; + } + + for (i = 0; i < nfds ; i++) { + if (fds[i].fd == errfd || + fds[i].fd == outfd) { + char data[1024]; + char **buf; + size_t *len; + int done; + if (fds[i].fd == outfd) { + buf = cmd->outbuf; + len = &outlen; + } else { + buf = cmd->errbuf; + len = &errlen; + } + + done = read(fds[i].fd, data, sizeof(data)); + if (done < 0) { + if (errno != EINTR && + errno != EAGAIN) { + virReportSystemError(errno, "%s", + _("unable to write to child input")); + return -1; + } + } else if (done == 0) { + if (fds[i].fd == outfd) + outfd = -1; + else + errfd = -1; + } else { + if (VIR_REALLOC_N(*buf, *len + done + 1) < 0) { + virReportOOMError(); + return -1; + } + memcpy(*buf + *len, data, done); + *len += done; + (*buf)[*len] = '\0'; + } + } else { + int done; + + done = write(infd, cmd->inbuf + inoff, + inlen - inoff); + if (done < 0) { + if (errno != EINTR && + errno != EAGAIN) { + virReportSystemError(errno, "%s", + _("unable to write to child input")); + return -1; + } + } else { + inoff += done; + if (inoff == inlen) { + int tmpfd = infd; + if (VIR_CLOSE(infd) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + } + } + } + + } + } + + return 0; +} + + +/* + * Run the command and wait for completion. + * Returns -1 on any error executing the + * command. Returns 0 if the command executed, + * with the exit status set + */ +int +virCommandRun(virCommandPtr cmd, int *exitstatus) +{ + int ret = 0; + char *outbuf = NULL; + char *errbuf = NULL; + int infd[2]; + + if (!cmd || cmd->has_error == -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return -1; + } + if (cmd->has_error == ENOMEM) { + virReportOOMError(); + return -1; + } + + /* If we have an input buffer, we need + * a pipe to feed the data to the child */ + if (cmd->inbuf) { + if (pipe(infd) < 0) { + virReportSystemError(errno, "%s", + _("unable to open pipe")); + return -1; + } + cmd->infd = infd[0]; + cmd->inpipe = infd[1]; + } + + if (virCommandRunAsync(cmd, NULL) < 0) { + if (cmd->inbuf) { + int tmpfd = infd[0]; + if (VIR_CLOSE(infd[0]) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + tmpfd = infd[1]; + if (VIR_CLOSE(infd[1]) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + } + return -1; + } + + /* If caller hasn't requested capture of + * stdout/err, then capture it ourselves + * so we can log it + */ + if (!cmd->outbuf && + !cmd->outfdptr) { + cmd->outfd = -1; + cmd->outfdptr = &cmd->outfd; + cmd->outbuf = &outbuf; + } + if (!cmd->errbuf && + !cmd->errfdptr) { + cmd->errfd = -1; + cmd->errfdptr = &cmd->errfd; + cmd->errbuf = &errbuf; + } + + if (cmd->inbuf || cmd->outbuf || cmd->errbuf) + ret = virCommandProcessIO(cmd); + + if (virCommandWait(cmd, exitstatus) < 0) + ret = -1; + + VIR_DEBUG("Result stdout: '%s' stderr: '%s'", + NULLSTR(*cmd->outbuf), + NULLSTR(*cmd->errbuf)); + + /* Reset any capturing, in case caller runs + * this identical command again */ + if (cmd->inbuf) { + int tmpfd = infd[0]; + if (VIR_CLOSE(infd[0]) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + tmpfd = infd[1]; + if (VIR_CLOSE(infd[1]) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + } + if (cmd->outbuf == &outbuf) { + int tmpfd = cmd->outfd; + if (VIR_CLOSE(cmd->outfd) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + cmd->outfdptr = NULL; + cmd->outbuf = NULL; + } + if (cmd->errbuf == &errbuf) { + int tmpfd = cmd->errfd; + if (VIR_CLOSE(cmd->errfd) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + cmd->errfdptr = NULL; + cmd->errbuf = NULL; + } + + return ret; +} + + +/* + * Perform all virCommand-specific actions, along with the user hook. + */ +static int +virCommandHook(void *data) +{ + virCommandPtr cmd = data; + int res = 0; + + if (cmd->hook) + res = cmd->hook(cmd->opaque); + if (res == 0 && cmd->pwd) { + VIR_DEBUG("Running child in %s", cmd->pwd); + res = chdir(cmd->pwd); + } + return res; +} + + +/* + * Run the command asynchronously + * Returns -1 on any error executing the + * command. Returns 0 if the command executed. + */ +int +virCommandRunAsync(virCommandPtr cmd, pid_t *pid) +{ + int ret; + char *str; + + if (!cmd || cmd->has_error == -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return -1; + } + if (cmd->has_error == ENOMEM) { + virReportOOMError(); + return -1; + } + + if (cmd->pid != -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, + _("command is already running as pid %d"), + cmd->pid); + return -1; + } + + if (cmd->pwd && (cmd->flags & VIR_EXEC_DAEMON)) { + virCommandError(VIR_ERR_INTERNAL_ERROR, + _("daemonized command cannot set working directory %s"), + cmd->pwd); + return -1; + } + + + str = virArgvToString((const char *const *)cmd->args); + VIR_DEBUG("About to run %s", str ? str : cmd->args[0]); + VIR_FREE(str); + + ret = virExecWithHook((const char *const *)cmd->args, + (const char *const *)cmd->env, + &cmd->preserve, + &cmd->pid, + cmd->infd, + cmd->outfdptr, + cmd->errfdptr, + cmd->flags, + virCommandHook, + cmd, + cmd->pidfile); + + VIR_DEBUG("Command result %d, with PID %d", + ret, (int)cmd->pid); + + if (ret == 0 && pid) + *pid = cmd->pid; + + return ret; +} + + +/* + * Wait for the async command to complete. + * Return -1 on any error waiting for + * completion. Returns 0 if the command + * finished with the exit status set + */ +int +virCommandWait(virCommandPtr cmd, int *exitstatus) +{ + int ret; + int status; + + if (!cmd || cmd->has_error == -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return -1; + } + if (cmd->has_error == ENOMEM) { + virReportOOMError(); + return -1; + } + + if (cmd->pid == -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("command is not yet running")); + return -1; + } + + + /* Wait for intermediate process to exit */ + while ((ret = waitpid(cmd->pid, &status, 0)) == -1 && + errno == EINTR); + + if (ret == -1) { + virReportSystemError(errno, _("unable to wait for process %d"), + cmd->pid); + return -1; + } + + cmd->pid = -1; + + if (exitstatus == NULL) { + if (status != 0) { + virCommandError(VIR_ERR_INTERNAL_ERROR, + _("Intermediate daemon process exited with status %d."), + WEXITSTATUS(status)); + return -1; + } + } else { + *exitstatus = status; + } + + return 0; +} + + +/* + * Release all resources + */ +void +virCommandFree(virCommandPtr cmd) +{ + int i; + if (!cmd) + return; + + VIR_FORCE_CLOSE(cmd->outfd); + VIR_FORCE_CLOSE(cmd->errfd); + + for (i = 0 ; i < cmd->nargs ; i++) + VIR_FREE(cmd->args[i]); + VIR_FREE(cmd->args); + + for (i = 0 ; i < cmd->nenv ; i++) + VIR_FREE(cmd->env[i]); + VIR_FREE(cmd->env); + + VIR_FREE(cmd->pwd); + + VIR_FREE(cmd->pidfile); + + VIR_FREE(cmd); +} diff --git a/src/util/command.h b/src/util/command.h new file mode 100644 index 0000000..ca24869 --- /dev/null +++ b/src/util/command.h @@ -0,0 +1,213 @@ +/* + * command.h: Child command execution + * + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#ifndef __VIR_COMMAND_H__ +# define __VIR_COMMAND_H__ + +# include "internal.h" +# include "util.h" + +typedef struct _virCommand virCommand; +typedef virCommand *virCommandPtr; + +/* + * Create a new command for named binary + */ +virCommandPtr virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1); + +/* + * Create a new command with a NULL terminated + * set of args, taking binary from argv[0] + */ +virCommandPtr virCommandNewArgs(const char *const*args) ATTRIBUTE_NONNULL(1); + +/* All error report from these setup APIs is + * delayed until the Run/Exec/Wait methods + */ + +/* + * Preserve the specified file descriptor + * in the child, instead of closing it + */ +void virCommandPreserveFD(virCommandPtr cmd, + int fd); + +/* + * Save the child PID in a pidfile + */ +void virCommandSetPidFile(virCommandPtr cmd, + const char *pidfile) ATTRIBUTE_NONNULL(2); + +/* + * Remove all capabilities from the child + */ +void virCommandClearCaps(virCommandPtr cmd); + +# if 0 +/* + * Re-allow a specific capability + */ +void virCommandAllowCap(virCommandPtr cmd, + int capability); +# endif + +/* + * Daemonize the child process + */ +void virCommandDaemonize(virCommandPtr cmd); + + +/* + * Add an environment variable to the child + * using separate name & value strings + */ +void virCommandAddEnvPair(virCommandPtr cmd, + const char *name, + const char *value) ATTRIBUTE_NONNULL(2); + +/* + * Add an environemnt variable to the child + * using a preformated env string FOO=BAR + */ +void virCommandAddEnvString(virCommandPtr cmd, + const char *str) ATTRIBUTE_NONNULL(2); +/* + * Pass an environment variable to the child + * using current process' value + */ +void virCommandAddEnvPass(virCommandPtr cmd, + const char *name) ATTRIBUTE_NONNULL(2); +/* + * Pass a common set of environment variables + * to the child using current process' values + */ +void virCommandAddEnvPassCommon(virCommandPtr cmd); + +/* + * Add a command line argument to the child + */ +void virCommandAddArg(virCommandPtr cmd, + const char *val) ATTRIBUTE_NONNULL(2); +/* + * Add a command line argument to the child + */ +void virCommandAddArgPair(virCommandPtr cmd, + const char *name, + const char *val) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +/* + * Add a NULL terminated array of args + */ +void virCommandAddArgSet(virCommandPtr cmd, + const char *const*vals) ATTRIBUTE_NONNULL(2); +/* + * Add a NULL terminated list of args + */ +void virCommandAddArgList(virCommandPtr cmd, + ... /* const char *arg, ..., NULL */) + ATTRIBUTE_SENTINEL; + +/* + * Set the working directory of a non-daemon child process, rather + * than the parent's working directory. Daemons automatically get / + * without using this call. + */ +void virCommandSetWorkingDirectory(virCommandPtr cmd, + const char *pwd) ATTRIBUTE_NONNULL(2); + +/* + * Feed the child's stdin from a string buffer. + * + * NB: Only works with virCommandRun() + */ +void virCommandSetInputBuffer(virCommandPtr cmd, + const char *inbuf) ATTRIBUTE_NONNULL(2); +/* + * Capture the child's stdout to a string buffer + * + * NB: Only works with virCommandRun() + */ +void virCommandSetOutputBuffer(virCommandPtr cmd, + char **outbuf) ATTRIBUTE_NONNULL(2); +/* + * Capture the child's stderr to a string buffer + * + * NB: Only works with virCommandRun() + */ +void virCommandSetErrorBuffer(virCommandPtr cmd, + char **errbuf) ATTRIBUTE_NONNULL(2); + +/* + * Set a file descriptor as the child's stdin + */ +void virCommandSetInputFD(virCommandPtr cmd, + int infd); +/* + * Set a file descriptor as the child's stdout + */ +void virCommandSetOutputFD(virCommandPtr cmd, + int *outfd) ATTRIBUTE_NONNULL(2); +/* + * Set a file descriptor as the child's stderr + */ +void virCommandSetErrorFD(virCommandPtr cmd, + int *errfd) ATTRIBUTE_NONNULL(2); + +/* + * A hook function to run between fork + exec + */ +void virCommandSetPreExecHook(virCommandPtr cmd, + virExecHook hook, + void *opaque) ATTRIBUTE_NONNULL(2); + +/* + * Run the command and wait for completion. + * Returns -1 on any error executing the + * command. Returns 0 if the command executed, + * with the exit status set + */ +int virCommandRun(virCommandPtr cmd, + int *exitstatus) ATTRIBUTE_RETURN_CHECK; + +/* + * Run the command asynchronously + * Returns -1 on any error executing the + * command. Returns 0 if the command executed. + */ +int virCommandRunAsync(virCommandPtr cmd, + pid_t *pid) ATTRIBUTE_RETURN_CHECK; + +/* + * Wait for the async command to complete. + * Return -1 on any error waiting for + * completion. Returns 0 if the command + * finished with the exit status set + */ +int virCommandWait(virCommandPtr cmd, + int *exitstatus) ATTRIBUTE_RETURN_CHECK; + +/* + * Release all resources + */ +void virCommandFree(virCommandPtr cmd); + + +#endif /* __VIR_COMMAND_H__ */ diff --git a/tests/.gitignore b/tests/.gitignore index 8ad3e98..e3906f0 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -1,6 +1,10 @@ *.exe .deps .libs +commandhelper +commandhelper.log +commandhelper.pid +commandtest conftest esxutilstest eventtest diff --git a/tests/Makefile.am b/tests/Makefile.am index 77b6fb9..ff3f135 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -72,7 +72,8 @@ EXTRA_DIST = \ qemuhelpdata check_PROGRAMS = virshtest conftest sockettest \ - nodeinfotest qparamtest virbuftest + nodeinfotest qparamtest virbuftest \ + commandtest commandhelper if WITH_XEN check_PROGRAMS += xml2sexprtest sexpr2xmltest \ @@ -154,6 +155,7 @@ TESTS = virshtest \ qparamtest \ virbuftest \ sockettest \ + commandtest \ $(test_scripts) if WITH_XEN @@ -339,6 +341,20 @@ nodeinfotest_SOURCES = \ nodeinfotest.c testutils.h testutils.c nodeinfotest_LDADD = $(LDADDS) +commandtest_SOURCES = \ + commandtest.c testutils.h testutils.c +commandtest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" +commandtest_LDADD = $(LDADDS) + +commandhelper_SOURCES = \ + commandhelper.c +commandhelper_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" +commandhelper_LDADD = $(LDADDS) + +statstest_SOURCES = \ + statstest.c testutils.h testutils.c +statstest_LDADD = $(LDADDS) + if WITH_SECDRIVER_SELINUX seclabeltest_SOURCES = \ seclabeltest.c diff --git a/tests/commanddata/test10.log b/tests/commanddata/test10.log new file mode 100644 index 0000000..e1d6092 --- /dev/null +++ b/tests/commanddata/test10.log @@ -0,0 +1,14 @@ +ARG:-version +ARG:-log=bar.log +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test11.log b/tests/commanddata/test11.log new file mode 100644 index 0000000..e1d6092 --- /dev/null +++ b/tests/commanddata/test11.log @@ -0,0 +1,14 @@ +ARG:-version +ARG:-log=bar.log +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test12.log b/tests/commanddata/test12.log new file mode 100644 index 0000000..1b59206 --- /dev/null +++ b/tests/commanddata/test12.log @@ -0,0 +1,12 @@ +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test13.log b/tests/commanddata/test13.log new file mode 100644 index 0000000..1b59206 --- /dev/null +++ b/tests/commanddata/test13.log @@ -0,0 +1,12 @@ +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test14.log b/tests/commanddata/test14.log new file mode 100644 index 0000000..1b59206 --- /dev/null +++ b/tests/commanddata/test14.log @@ -0,0 +1,12 @@ +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test15.log b/tests/commanddata/test15.log new file mode 100644 index 0000000..f439a85 --- /dev/null +++ b/tests/commanddata/test15.log @@ -0,0 +1,12 @@ +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:.../commanddata diff --git a/tests/commanddata/test2.log b/tests/commanddata/test2.log new file mode 100644 index 0000000..1b59206 --- /dev/null +++ b/tests/commanddata/test2.log @@ -0,0 +1,12 @@ +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test3.log b/tests/commanddata/test3.log new file mode 100644 index 0000000..6bd7974 --- /dev/null +++ b/tests/commanddata/test3.log @@ -0,0 +1,14 @@ +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +FD:3 +FD:5 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test4.log b/tests/commanddata/test4.log new file mode 100644 index 0000000..1876685 --- /dev/null +++ b/tests/commanddata/test4.log @@ -0,0 +1,12 @@ +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:yes +CWD:/ diff --git a/tests/commanddata/test5.log b/tests/commanddata/test5.log new file mode 100644 index 0000000..f745c3f --- /dev/null +++ b/tests/commanddata/test5.log @@ -0,0 +1,10 @@ +ENV:HOME=/home/test +ENV:LC_ALL=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test6.log b/tests/commanddata/test6.log new file mode 100644 index 0000000..5394428 --- /dev/null +++ b/tests/commanddata/test6.log @@ -0,0 +1,6 @@ +ENV:DISPLAY=:0.0 +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test7.log b/tests/commanddata/test7.log new file mode 100644 index 0000000..cdfe445 --- /dev/null +++ b/tests/commanddata/test7.log @@ -0,0 +1,11 @@ +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:LC_ALL=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test8.log b/tests/commanddata/test8.log new file mode 100644 index 0000000..87874fd --- /dev/null +++ b/tests/commanddata/test8.log @@ -0,0 +1,7 @@ +ENV:LANG=C +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commanddata/test9.log b/tests/commanddata/test9.log new file mode 100644 index 0000000..2607530 --- /dev/null +++ b/tests/commanddata/test9.log @@ -0,0 +1,18 @@ +ARG:-version +ARG:-log=bar.log +ARG:arg1 +ARG:arg2 +ARG:arg3 +ARG:arg4 +ENV:DISPLAY=:0.0 +ENV:HOME=/home/test +ENV:HOSTNAME=test +ENV:LANG=C +ENV:LOGNAME=testTMPDIR=/tmp +ENV:PATH=/usr/bin:/bin +ENV:USER=test +FD:0 +FD:1 +FD:2 +DAEMON:no +CWD:/tmp diff --git a/tests/commandhelper.c b/tests/commandhelper.c new file mode 100644 index 0000000..dc8998f --- /dev/null +++ b/tests/commandhelper.c @@ -0,0 +1,136 @@ +/* + * commandhelper.c: Auxiliary program for commandtest + * + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#include <config.h> + +#include <stdio.h> +#include <unistd.h> +#include <stdlib.h> +#include <fcntl.h> +#include <string.h> + +#include "internal.h" +#include "util.h" +#include "memory.h" + + +static int envsort(const void *a, const void *b) { + const char *const*astrptr = a; + const char *const*bstrptr = b; + const char *astr = *astrptr; + const char *bstr = *bstrptr; + char *aeq = strchr(astr, '='); + char *beq = strchr(bstr, '='); + char *akey = strndup(astr, aeq - astr); + char *bkey = strndup(bstr, beq - bstr); + int ret = strcmp(akey, bkey); + free(akey); + free(bkey); + return ret; +} + +int main(int argc, char **argv) { + int i, n; + char **origenv; + char **newenv; + FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); + + if (!log) + goto error; + + for (i = 1 ; i < argc ; i++) { + fprintf(log, "ARG:%s\n", argv[i]); + } + + origenv = environ; + n = 0; + while (*origenv != NULL) { + n++; + origenv++; + } + + if (VIR_ALLOC_N(newenv, n) < 0) { + exit(EXIT_FAILURE); + } + + origenv = environ; + n = i = 0; + while (*origenv != NULL) { + newenv[i++] = *origenv; + n++; + origenv++; + } + qsort(newenv, n, sizeof(newenv[0]), envsort); + + for (i = 0 ; i < n ; i++) { + fprintf(log, "ENV:%s\n", newenv[i]); + } + + for (i = 0 ; i < sysconf(_SC_OPEN_MAX) ; i++) { + int f; + int closed; + if (i == fileno(log)) + continue; + closed = fcntl(i, F_GETFD, &f) == -1 && + errno == EBADF; + if (!closed) + fprintf(log, "FD:%d\n", i); + } + + fprintf(log, "DAEMON:%s\n", getppid() == 1 ? "yes" : "no"); + char cwd[1024]; + getcwd(cwd, sizeof(cwd)); + if (strlen(cwd) > strlen("/commanddata") && + STREQ(cwd + strlen(cwd) - strlen("/commanddata"), "/commanddata")) + strcpy(cwd, ".../commanddata"); + fprintf(log, "CWD:%s\n", cwd); + + fclose(log); + + char buf[1024]; + ssize_t got; + + fprintf(stdout, "BEGIN STDOUT\n"); + fflush(stdout); + fprintf(stderr, "BEGIN STDERR\n"); + fflush(stderr); + + for (;;) { + got = read(STDIN_FILENO, buf, sizeof(buf)); + if (got < 0) + goto error; + if (got == 0) + break; + if (safewrite(STDOUT_FILENO, buf, got) != got) + goto error; + if (safewrite(STDERR_FILENO, buf, got) != got) + goto error; + } + + fprintf(stdout, "END STDOUT\n"); + fflush(stdout); + fprintf(stderr, "END STDERR\n"); + fflush(stderr); + + return EXIT_SUCCESS; + +error: + return EXIT_FAILURE; +} diff --git a/tests/commandtest.c b/tests/commandtest.c new file mode 100644 index 0000000..5479bfc --- /dev/null +++ b/tests/commandtest.c @@ -0,0 +1,572 @@ +/* + * commandtest.c: Test the libCommand API + * + * Copyright (C) 2010 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <signal.h> + +#include "testutils.h" +#include "internal.h" +#include "nodeinfo.h" +#include "util.h" +#include "memory.h" +#include "command.h" + +#ifndef __linux__ + +static int +mymain(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) +{ + exit (EXIT_AM_SKIP); +} + +#else + +static char *progname; +static char *abs_srcdir; + + +static int checkoutput(const char *testname) { + int ret = -1; + char cwd[1024]; + char *expectname = NULL; + char *expectlog = NULL; + char *actualname = NULL; + char *actuallog = NULL; + + if (!getcwd(cwd, sizeof(cwd))) + return -1; + + if (virAsprintf(&expectname, "%s/commanddata/%s.log", abs_srcdir, + testname) < 0) + goto cleanup; + if (virAsprintf(&actualname, "%s/commandhelper.log", abs_builddir) < 0) + goto cleanup; + + if (virFileReadAll(expectname, 1024*64, &expectlog) < 0) { + fprintf(stderr, "cannot read %s\n", expectname); + goto cleanup; + } + + if (virFileReadAll(actualname, 1024*64, &actuallog) < 0) { + fprintf(stderr, "cannot read %s\n", actualname); + goto cleanup; + } + + if (STRNEQ(expectlog, actuallog)) { + virtTestDifference(stderr, expectlog, actuallog); + goto cleanup; + } + + + ret = 0; + +cleanup: + unlink(actuallog); + VIR_FREE(actuallog); + VIR_FREE(actualname); + VIR_FREE(expectlog); + VIR_FREE(expectname); + return ret; +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * Only stdin/out/err open + * No slot for return status must log error. + */ +static int test0(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd; + char *log; + int ret = -1; + + free(virtTestLogContentAndReset()); + cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); + if (virCommandRun(cmd, NULL) == 0) + goto cleanup; + if ((log = virtTestLogContentAndReset()) == NULL) + goto cleanup; + if (strstr(log, ": error :") == NULL) + goto cleanup; + virResetLastError(); + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * Only stdin/out/err open + * Capturing return status must not log error. + */ +static int test1(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd; + int ret = -1; + int status; + + cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); + if (virCommandRun(cmd, &status) < 0) + goto cleanup; + if (status == 0) + goto cleanup; + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * Only stdin/out/err open + */ +static int test2(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test2"); +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * stdin/out/err + two extra FD open + */ +static int test3(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + int newfd1 = dup(STDERR_FILENO); + int newfd2 = dup(STDERR_FILENO); + int newfd3 = dup(STDERR_FILENO); + close(newfd2); + + virCommandPreserveFD(cmd, newfd1); + virCommandPreserveFD(cmd, newfd3); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test3"); +} + + +/* + * Run program, no args, inherit all ENV, CWD is / + * Only stdin/out/err open. + * Daemonized + */ +static int test4(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + pid_t pid; + char *pidfile = virFilePid(abs_builddir, "commandhelper"); + + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + if (virFileReadPid(abs_builddir, "commandhelper", &pid) != 0) { + printf("cannot read pidfile\n"); + return -1; + } + while (kill(pid, 0) != -1) + usleep(100*1000); + + virCommandFree(cmd); + + VIR_FREE(pidfile); + + return checkoutput("test4"); +} + + +/* + * Run program, no args, inherit filtered ENV, keep CWD. + * Only stdin/out/err open + */ +static int test5(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + virCommandAddEnvPassCommon(cmd); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test5"); +} + + +/* + * Run program, no args, inherit filtered ENV, keep CWD. + * Only stdin/out/err open + */ +static int test6(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + virCommandAddEnvPass(cmd, "DISPLAY"); + virCommandAddEnvPass(cmd, "DOESNOTEXIST"); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test6"); +} + + +/* + * Run program, no args, inherit filtered ENV, keep CWD. + * Only stdin/out/err open + */ +static int test7(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + virCommandAddEnvPassCommon(cmd); + virCommandAddEnvPass(cmd, "DISPLAY"); + virCommandAddEnvPass(cmd, "DOESNOTEXIST"); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test7"); +} + +/* + * Run program, no args, inherit filtered ENV, keep CWD. + * Only stdin/out/err open + */ +static int test8(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + virCommandAddEnvString(cmd, "LANG=C"); + virCommandAddEnvPair(cmd, "USER", "test"); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test8"); +} + + +/* + * Run program, some args, inherit all ENV, keep CWD. + * Only stdin/out/err open + */ +static int test9(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + const char* const args[] = { "arg1", "arg2", NULL }; + + virCommandAddArg(cmd, "-version"); + virCommandAddArgPair(cmd, "-log", "bar.log"); + virCommandAddArgSet(cmd, args); + virCommandAddArgList(cmd, "arg3", "arg4", NULL); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test9"); +} + + +/* + * Run program, some args, inherit all ENV, keep CWD. + * Only stdin/out/err open + */ +static int test10(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + const char *const args[] = { + "-version", "-log=bar.log", NULL, + }; + + virCommandAddArgSet(cmd, args); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test10"); +} + +/* + * Run program, some args, inherit all ENV, keep CWD. + * Only stdin/out/err open + */ +static int test11(const void *unused ATTRIBUTE_UNUSED) { + const char *args[] = { + abs_builddir "/commandhelper", + "-version", "-log=bar.log", NULL, + }; + virCommandPtr cmd = virCommandNewArgs(args); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test11"); +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * Only stdin/out/err open. Set stdin data + */ +static int test12(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + virCommandSetInputBuffer(cmd, "Hello World\n"); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test12"); +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * Only stdin/out/err open. Set stdin data + */ +static int test13(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + char *outactual = NULL; + const char *outexpect = "BEGIN STDOUT\n" + "Hello World\n" + "END STDOUT\n"; + int ret = -1; + + virCommandSetInputBuffer(cmd, "Hello World\n"); + virCommandSetOutputBuffer(cmd, &outactual); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + if (!STREQ(outactual, outexpect)) { + virtTestDifference(stderr, outactual, outexpect); + goto cleanup; + } + + if (checkoutput("test13") < 0) + goto cleanup; + + ret = 0; + +cleanup: + VIR_FREE(outactual); + return ret; +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * Only stdin/out/err open. Set stdin data + */ +static int test14(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + char *outactual = NULL; + const char *outexpect = "BEGIN STDOUT\n" + "Hello World\n" + "END STDOUT\n"; + char *erractual = NULL; + const char *errexpect = "BEGIN STDERR\n" + "Hello World\n" + "END STDERR\n"; + int ret = -1; + + virCommandSetInputBuffer(cmd, "Hello World\n"); + virCommandSetOutputBuffer(cmd, &outactual); + virCommandSetErrorBuffer(cmd, &erractual); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + if (!STREQ(outactual, outexpect)) { + virtTestDifference(stderr, outactual, outexpect); + goto cleanup; + } + if (!STREQ(erractual, errexpect)) { + virtTestDifference(stderr, erractual, errexpect); + goto cleanup; + } + + if (checkoutput("test14") < 0) + goto cleanup; + + ret = 0; + +cleanup: + VIR_FREE(outactual); + VIR_FREE(erractual); + return ret; +} + + +/* + * Run program, no args, inherit all ENV, change CWD. + * Only stdin/out/err open + */ +static int test15(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + virCommandSetWorkingDirectory(cmd, abs_builddir "/commanddata"); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test15"); +} + +static int +mymain(int argc, char **argv) +{ + int ret = 0; + char cwd[PATH_MAX]; + + abs_srcdir = getenv("abs_srcdir"); + if (!abs_srcdir) + abs_srcdir = getcwd(cwd, sizeof(cwd)); + + progname = argv[0]; + + if (argc > 1) { + fprintf(stderr, "Usage: %s\n", progname); + return(EXIT_FAILURE); + } + + if (chdir("/tmp") < 0) + return(EXIT_FAILURE); + + virInitialize(); + + const char *const newenv[] = { + "PATH=/usr/bin:/bin", + "HOSTNAME=test", + "LANG=C", + "HOME=/home/test", + "USER=test", + "LOGNAME=test" + "TMPDIR=/tmp", + "DISPLAY=:0.0", + NULL + }; + environ = (char **)newenv; + +# define DO_TEST(NAME) \ + if (virtTestRun("Command Exec " #NAME " test", \ + 1, NAME, NULL) < 0) \ + ret = -1 + + char *actualname; + if (virAsprintf(&actualname, "%s/commandhelper.log", abs_builddir) < 0) + return EXIT_FAILURE; + unlink(actualname); + VIR_FREE(actualname); + + DO_TEST(test0); + DO_TEST(test1); + DO_TEST(test2); + DO_TEST(test3); + DO_TEST(test4); + DO_TEST(test5); + DO_TEST(test6); + DO_TEST(test7); + DO_TEST(test8); + DO_TEST(test9); + DO_TEST(test10); + DO_TEST(test11); + DO_TEST(test12); + DO_TEST(test13); + DO_TEST(test14); + DO_TEST(test15); + + return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +#endif /* __linux__ */ + +VIRT_TEST_MAIN(mymain) -- 1.7.3.2

On Mon, Nov 22, 2010 at 03:30:35PM -0700, Eric Blake wrote: Ah yes, I forgot to mention that - it came in useful when converting the QEMU driver to the new APIs. How about having two separate methods ? virCommandPreserveFD virCommandTransferFD Means you don't have to remember whether true means transfer or don't transfer I think we also need duplicate this in virCommandFree(), because there may be scenarios where we get 1/2 way through populating a virCommandPtr instance, and then some other error means we have to stop & free it, without ever getting to virCommandRunAsync. Daniel