[libvirt] [PATCH 0/6] Refactor critical section in virDomainListAllDomains and fix qemuConnectGetAllDomainStats
Peter Krempa (6): util: Make the virDomainListFree helper more universal conf: Extract code to filter domain list into a separate function conf: Rename virDomainObjListFilter type to virDomainObjListACLFilter conf: Refactor domain list collection critical section conf: Add helper to convert list of virDomains to a list of virDomainObjs qemu: Convert qemuConnectGetAllDomainStats to use new helpers daemon/remote.c | 2 +- src/conf/domain_conf.c | 258 ++++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 29 +++-- src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 86 +++++++-------- src/util/virobject.c | 41 +++++++ src/util/virobject.h | 2 + tools/virsh-domain-monitor.c | 2 +- 8 files changed, 275 insertions(+), 150 deletions(-) -- 2.3.5
Extend it to a universal helper used for clearing lists of any objects. Note that the argument type is specifically void * to allow implicit typecasting. Additionally add a helper that works on non-NULL terminated arrays once we know the length. --- daemon/remote.c | 2 +- src/conf/domain_conf.c | 24 +----------------------- src/conf/domain_conf.h | 2 -- src/libvirt_private.syms | 3 ++- src/qemu/qemu_driver.c | 2 +- src/util/virobject.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 2 ++ tools/virsh-domain-monitor.c | 2 +- 8 files changed, 49 insertions(+), 29 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 3a3f168..e259a76 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -6382,7 +6382,7 @@ remoteDispatchConnectGetAllDomainStats(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); virDomainStatsRecordListFree(retStats); - virDomainListFree(doms); + virObjectListFree(doms); return rv; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5f74ab1..b9c4c61 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23033,28 +23033,6 @@ virDomainListPopulate(void *payload, #undef MATCH -/** - * virDomainListFree: - * @list: list of domains to free - * - * Frees a NULL-terminated list of domains without messing with currently - * set libvirt errors. - */ -void -virDomainListFree(virDomainPtr *list) -{ - virDomainPtr *next; - - if (!list) - return; - - for (next = list; *next; next++) - virObjectUnref(*next); - - VIR_FREE(list); -} - - int virDomainObjListExport(virDomainObjListPtr doms, virConnectPtr conn, @@ -23090,7 +23068,7 @@ virDomainObjListExport(virDomainObjListPtr doms, ret = data.ndomains; cleanup: - virDomainListFree(data.domains); + virObjectListFree(data.domains); virObjectUnlock(doms); return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 96988ef..11e4bb9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3054,8 +3054,6 @@ int virDomainObjListExport(virDomainObjListPtr doms, virDomainObjListFilter filter, unsigned int flags); -void virDomainListFree(virDomainPtr *list); - int virDomainDefMaybeAddController(virDomainDefPtr def, int type, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c8e6fb4..d7cac20 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -341,7 +341,6 @@ virDomainLifecycleCrashTypeFromString; virDomainLifecycleCrashTypeToString; virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; -virDomainListFree; virDomainLiveConfigHelperMethod; virDomainLoaderDefFree; virDomainLoaderTypeFromString; @@ -1881,6 +1880,8 @@ virClassNew; virObjectFreeCallback; virObjectFreeHashData; virObjectIsClass; +virObjectListFree; +virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectNew; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3695b26..3032b7a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19909,7 +19909,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, virDomainObjEndAPI(&dom); virDomainStatsRecordListFree(tmpstats); - virDomainListFree(domlist); + virObjectListFree(domlist); return ret; } diff --git a/src/util/virobject.c b/src/util/virobject.c index 9ccd310..51876b9 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -405,3 +405,44 @@ void virObjectFreeHashData(void *opaque, const void *name ATTRIBUTE_UNUSED) { virObjectUnref(opaque); } + + +/** + * virObjectListFree: + * @list: A pointer to a NULL-terminated list of object pointers to free + * + * Unrefs all members of @list and frees the list itself. + */ +void virObjectListFree(void *list) +{ + void **next; + + if (!list) + return; + + for (next = (void **) list; *next; next++) + virObjectUnref(*next); + + VIR_FREE(list); +} + + +/** + * virObjectListFreeCount: + * @list: A pointer to a list of object pointers to freea + * @count: Number of elements in the list. + * + * Unrefs all members of @list and frees the list itself. + */ +void virObjectListFreeCount(void *list, size_t count) +{ + size_t i; + + if (!list) + return; + + for (i = 0; i < count; i++) + virObjectUnref(((void **)list)[i]); + + VIR_FREE(list); +} diff --git a/src/util/virobject.h b/src/util/virobject.h index ad1f0c1..c3ecc1e 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -99,5 +99,7 @@ void virObjectLock(void *lockableobj) void virObjectUnlock(void *lockableobj) ATTRIBUTE_NONNULL(1); +void virObjectListFree(void *list); +void virObjectListFreeCount(void *list, size_t count); #endif /* __VIR_OBJECT_H */ diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 9686531..91c57e2 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -2193,7 +2193,7 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: virDomainStatsRecordListFree(records); - virDomainListFree(domlist); + virObjectListFree(domlist); return ret; } -- 2.3.5
Separate the code to simplify future refactors. --- src/conf/domain_conf.c | 82 +++++++++++++++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 35 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b9c4c61..057602b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22927,43 +22927,19 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, return ret; } -struct virDomainListData { - virConnectPtr conn; - virDomainPtr *domains; - virDomainObjListFilter filter; - unsigned int flags; - int ndomains; - bool error; -}; -#define MATCH(FLAG) (data->flags & (FLAG)) -static void -virDomainListPopulate(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) +#define MATCH(FLAG) (filter & (FLAG)) +static bool +virDomainObjMatchFilter(virDomainObjPtr vm, + unsigned int filter) { - struct virDomainListData *data = opaque; - virDomainObjPtr vm = payload; - virDomainPtr dom; - - if (data->error) - return; - - virObjectLock(vm); - /* check if the domain matches the filter */ - - /* filter by the callback function (access control checks) */ - if (data->filter != NULL && - !data->filter(data->conn, vm->def)) - goto cleanup; - /* filter by active state */ if (MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE) && !((MATCH(VIR_CONNECT_LIST_DOMAINS_ACTIVE) && virDomainObjIsActive(vm)) || (MATCH(VIR_CONNECT_LIST_DOMAINS_INACTIVE) && !virDomainObjIsActive(vm)))) - goto cleanup; + return false; /* filter by persistence */ if (MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT) && @@ -22971,7 +22947,7 @@ virDomainListPopulate(void *payload, vm->persistent) || (MATCH(VIR_CONNECT_LIST_DOMAINS_TRANSIENT) && !vm->persistent))) - goto cleanup; + return false; /* filter by domain state */ if (MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE)) { @@ -22986,7 +22962,7 @@ virDomainListPopulate(void *payload, (st != VIR_DOMAIN_RUNNING && st != VIR_DOMAIN_PAUSED && st != VIR_DOMAIN_SHUTOFF)))) - goto cleanup; + return false; } /* filter by existence of managed save state */ @@ -22995,22 +22971,59 @@ virDomainListPopulate(void *payload, vm->hasManagedSave) || (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE) && !vm->hasManagedSave))) - goto cleanup; + return false; /* filter by autostart option */ if (MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_AUTOSTART) && !((MATCH(VIR_CONNECT_LIST_DOMAINS_AUTOSTART) && vm->autostart) || (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART) && !vm->autostart))) - goto cleanup; + return false; /* filter by snapshot existence */ if (MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT)) { int nsnap = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0); if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT) && nsnap > 0) || (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT) && nsnap <= 0))) - goto cleanup; + return false; } + return true; +} +#undef MATCH + + +struct virDomainListData { + virConnectPtr conn; + virDomainPtr *domains; + virDomainObjListFilter filter; + unsigned int flags; + int ndomains; + bool error; +}; + +static void +virDomainListPopulate(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virDomainListData *data = opaque; + virDomainObjPtr vm = payload; + virDomainPtr dom; + + if (data->error) + return; + + virObjectLock(vm); + /* check if the domain matches the filter */ + + /* filter by the callback function (access control checks) */ + if (data->filter != NULL && + !data->filter(data->conn, vm->def)) + goto cleanup; + + if (!virDomainObjMatchFilter(vm, data->flags)) + goto cleanup; + /* just count the machines */ if (!data->domains) { data->ndomains++; @@ -23030,7 +23043,6 @@ virDomainListPopulate(void *payload, virObjectUnlock(vm); return; } -#undef MATCH int -- 2.3.5
The passed function is meant to filter domains according to ACL match. --- src/conf/domain_conf.c | 16 ++++++++-------- src/conf/domain_conf.h | 12 ++++++------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 057602b..c2b7d6a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22043,7 +22043,7 @@ virDomainGetFilesystemForTarget(virDomainDefPtr def, struct virDomainObjListData { - virDomainObjListFilter filter; + virDomainObjListACLFilter filter; virConnectPtr conn; bool active; int count; @@ -22074,7 +22074,7 @@ virDomainObjListCount(void *payload, int virDomainObjListNumOfDomains(virDomainObjListPtr doms, bool active, - virDomainObjListFilter filter, + virDomainObjListACLFilter filter, virConnectPtr conn) { struct virDomainObjListData data = { filter, conn, active, 0 }; @@ -22085,7 +22085,7 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms, } struct virDomainIDData { - virDomainObjListFilter filter; + virDomainObjListACLFilter filter; virConnectPtr conn; int numids; int maxids; @@ -22113,7 +22113,7 @@ int virDomainObjListGetActiveIDs(virDomainObjListPtr doms, int *ids, int maxids, - virDomainObjListFilter filter, + virDomainObjListACLFilter filter, virConnectPtr conn) { struct virDomainIDData data = { filter, conn, @@ -22125,7 +22125,7 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms, } struct virDomainNameData { - virDomainObjListFilter filter; + virDomainObjListACLFilter filter; virConnectPtr conn; int oom; int numnames; @@ -22163,7 +22163,7 @@ int virDomainObjListGetInactiveNames(virDomainObjListPtr doms, char **const names, int maxnames, - virDomainObjListFilter filter, + virDomainObjListACLFilter filter, virConnectPtr conn) { struct virDomainNameData data = { filter, conn, @@ -22995,7 +22995,7 @@ virDomainObjMatchFilter(virDomainObjPtr vm, struct virDomainListData { virConnectPtr conn; virDomainPtr *domains; - virDomainObjListFilter filter; + virDomainObjListACLFilter filter; unsigned int flags; int ndomains; bool error; @@ -23049,7 +23049,7 @@ int virDomainObjListExport(virDomainObjListPtr doms, virConnectPtr conn, virDomainPtr **domains, - virDomainObjListFilter filter, + virDomainObjListACLFilter filter, unsigned int flags) { int ret = -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 11e4bb9..bbfe96e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2318,8 +2318,8 @@ struct _virDomainObj { typedef struct _virDomainObjList virDomainObjList; typedef virDomainObjList *virDomainObjListPtr; -typedef bool (*virDomainObjListFilter)(virConnectPtr conn, - virDomainDefPtr def); +typedef bool (*virDomainObjListACLFilter)(virConnectPtr conn, + virDomainDefPtr def); /* This structure holds various callbacks and data needed @@ -2840,18 +2840,18 @@ unsigned int virDomainVideoDefaultRAM(const virDomainDef *def, int virDomainObjListNumOfDomains(virDomainObjListPtr doms, bool active, - virDomainObjListFilter filter, + virDomainObjListACLFilter filter, virConnectPtr conn); int virDomainObjListGetActiveIDs(virDomainObjListPtr doms, int *ids, int maxids, - virDomainObjListFilter filter, + virDomainObjListACLFilter filter, virConnectPtr conn); int virDomainObjListGetInactiveNames(virDomainObjListPtr doms, char **const names, int maxnames, - virDomainObjListFilter filter, + virDomainObjListACLFilter filter, virConnectPtr conn); typedef int (*virDomainObjListIterator)(virDomainObjPtr dom, @@ -3051,7 +3051,7 @@ VIR_ENUM_DECL(virDomainStartupPolicy) int virDomainObjListExport(virDomainObjListPtr doms, virConnectPtr conn, virDomainPtr **domains, - virDomainObjListFilter filter, + virDomainObjListACLFilter filter, unsigned int flags); int -- 2.3.5
Until now the virDomainListAllDomains API would lock the domain list and then every single domain object to access and filter it. This would potentially allow a unresponsive VM to block the whole daemon if a *listAllDomains call would get stuck. To avoid this problem this patch collects a list of referenced domain objects first from the list and then unlocks it right away. The expensive operation requiring locking of the domain object is executed after the list lock is dropped. While a single blocked domain will still lock up a listAllDomains call, the domain list won't be held locked and thus other APIs won't be blocked. Additionally this patch also fixes the lookup code, where we'd ignore the vm->removing flag and thus potentially return domain objects that would be deleted very soon so calling any API wouldn't make sense. As other clients also could benefit from operating on a list of domain objects rather than the public domain descriptors a new intermediate API - virDomainObjListCollect - is introduced by this patch. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1181074 --- src/conf/domain_conf.c | 142 +++++++++++++++++++++++++++++------------------ src/conf/domain_conf.h | 6 ++ src/libvirt_private.syms | 1 + 3 files changed, 95 insertions(+), 54 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c2b7d6a..66fe470 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22993,98 +22993,132 @@ virDomainObjMatchFilter(virDomainObjPtr vm, struct virDomainListData { - virConnectPtr conn; - virDomainPtr *domains; - virDomainObjListACLFilter filter; - unsigned int flags; - int ndomains; - bool error; + virDomainObjPtr *vms; + size_t nvms; }; + static void -virDomainListPopulate(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) +virDomainObjListCollectIterator(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { struct virDomainListData *data = opaque; - virDomainObjPtr vm = payload; - virDomainPtr dom; - if (data->error) - return; + data->vms[data->nvms++] = virObjectRef(payload); +} - virObjectLock(vm); - /* check if the domain matches the filter */ - /* filter by the callback function (access control checks) */ - if (data->filter != NULL && - !data->filter(data->conn, vm->def)) - goto cleanup; +static void +virDomainObjListFilter(virDomainObjPtr **list, + size_t *nvms, + virConnectPtr conn, + virDomainObjListACLFilter filter, + unsigned int flags) +{ + size_t i = 0; - if (!virDomainObjMatchFilter(vm, data->flags)) - goto cleanup; + while (i < *nvms) { + virDomainObjPtr vm = (*list)[i]; - /* just count the machines */ - if (!data->domains) { - data->ndomains++; - goto cleanup; + virObjectLock(vm); + + /* do not list the object if: + * 1) it's being removed. + * 2) connection does not have ACL to see it + * 3) it doesn't match the filter + */ + if (vm->removing || + (filter && !filter(conn, vm->def)) || + !virDomainObjMatchFilter(vm, flags)) { + virObjectUnlock(vm); + virObjectUnref(vm); + VIR_DELETE_ELEMENT(*list, i, *nvms); + continue; + } + + virObjectUnlock(vm); + i++; } +} - if (!(dom = virGetDomain(data->conn, vm->def->name, vm->def->uuid))) { - data->error = true; - goto cleanup; + +int +virDomainObjListCollect(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags) +{ + struct virDomainListData data = { NULL, 0 }; + + virObjectLock(domlist); + if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) { + virObjectUnlock(domlist); + return -1; } - dom->id = vm->def->id; + virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data); + virObjectUnlock(domlist); - data->domains[data->ndomains++] = dom; + virDomainObjListFilter(&data.vms, &data.nvms, conn, filter, flags); - cleanup: - virObjectUnlock(vm); - return; + *nvms = data.nvms; + *vms = data.vms; + + return 0; } int -virDomainObjListExport(virDomainObjListPtr doms, +virDomainObjListExport(virDomainObjListPtr domlist, virConnectPtr conn, virDomainPtr **domains, virDomainObjListACLFilter filter, unsigned int flags) { + virDomainObjPtr *vms = NULL; + virDomainPtr *doms = NULL; + size_t nvms = 0; + size_t i; int ret = -1; - struct virDomainListData data = { - conn, NULL, - filter, - flags, 0, false - }; + if (virDomainObjListCollect(domlist, conn, &vms, &nvms, filter, flags) < 0) + return -1; - virObjectLock(doms); - if (domains && - VIR_ALLOC_N(data.domains, virHashSize(doms->objs) + 1) < 0) - goto cleanup; + if (domains) { + if (VIR_ALLOC_N(doms, nvms + 1) < 0) + goto cleanup; - virHashForEach(doms->objs, virDomainListPopulate, &data); + for (i = 0; i < nvms; i++) { + virDomainObjPtr vm = vms[i]; - if (data.error) - goto cleanup; + virObjectLock(vm); + + if (!(doms[i] = virGetDomain(conn, vm->def->name, vm->def->uuid))) { + virObjectUnlock(vm); + goto cleanup; + } + + doms[i]->id = vm->def->id; + + virObjectUnlock(vm); + } - if (data.domains) { - /* trim the array to the final size */ - ignore_value(VIR_REALLOC_N(data.domains, data.ndomains + 1)); - *domains = data.domains; - data.domains = NULL; + *domains = doms; + doms = NULL; } - ret = data.ndomains; + ret = nvms; cleanup: - virObjectListFree(data.domains); - virObjectUnlock(doms); + virObjectListFree(doms); + virObjectListFreeCount(vms, nvms); return ret; } + virSecurityLabelDefPtr virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bbfe96e..ac11a20 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3048,6 +3048,12 @@ VIR_ENUM_DECL(virDomainStartupPolicy) VIR_CONNECT_LIST_DOMAINS_FILTERS_AUTOSTART | \ VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT) +int virDomainObjListCollect(virDomainObjListPtr doms, + virConnectPtr conn, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags); int virDomainObjListExport(virDomainObjListPtr doms, virConnectPtr conn, virDomainPtr **domains, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d7cac20..9a7a944 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -385,6 +385,7 @@ virDomainObjGetMetadata; virDomainObjGetPersistentDef; virDomainObjGetState; virDomainObjListAdd; +virDomainObjListCollect; virDomainObjListExport; virDomainObjListFindByID; virDomainObjListFindByName; -- 2.3.5
On 04/30/2015 08:44 AM, Peter Krempa wrote: ...
- if (!(dom = virGetDomain(data->conn, vm->def->name, vm->def->uuid))) { - data->error = true; - goto cleanup; + +int +virDomainObjListCollect(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags) +{ + struct virDomainListData data = { NULL, 0 }; + + virObjectLock(domlist); + if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
Coverity checker wasn't very happy with this especially if virHashSize returns a negative number. John
+ virObjectUnlock(domlist); + return -1; }
- dom->id = vm->def->id; + virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data); + virObjectUnlock(domlist);
- data->domains[data->ndomains++] = dom; + virDomainObjListFilter(&data.vms, &data.nvms, conn, filter, flags);
- cleanup: - virObjectUnlock(vm); - return; + *nvms = data.nvms; + *vms = data.vms; + + return 0; }
On Mon, May 11, 2015 at 06:29:35 -0400, John Ferlan wrote:
On 04/30/2015 08:44 AM, Peter Krempa wrote:
...
- if (!(dom = virGetDomain(data->conn, vm->def->name, vm->def->uuid))) { - data->error = true; - goto cleanup; + +int +virDomainObjListCollect(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags) +{ + struct virDomainListData data = { NULL, 0 }; + + virObjectLock(domlist); + if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
Coverity checker wasn't very happy with this especially if virHashSize returns a negative number.
False positive. virHashSize returns null only if the hash is NULL which never happens for domlist-objs. Peter
Add virDomainObjListConvert that will take a list of virDomains, apply filters and return a list of virDomainObjs. --- src/conf/domain_conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 64 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66fe470..73dc33f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23072,6 +23072,60 @@ virDomainObjListCollect(virDomainObjListPtr domlist, int +virDomainObjListConvert(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainPtr *doms, + size_t ndoms, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags, + bool skip_missing) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDomainObjPtr vm; + size_t i; + + *nvms = 0; + *vms = NULL; + + virObjectLock(domlist); + for (i = 0; i < ndoms; i++) { + virDomainPtr dom = doms[i]; + + virUUIDFormat(dom->uuid, uuidstr); + + if (!(vm = virHashLookup(domlist->objs, uuidstr))) { + if (!skip_missing) { + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->name); + virObjectUnlock(domlist); + goto error; + } + } else { + virObjectRef(vm); + + if (VIR_APPEND_ELEMENT(*vms, *nvms, vm) < 0) + goto error; + } + } + virObjectUnlock(domlist); + + virDomainObjListFilter(vms, nvms, conn, filter, flags); + + return 0; + + error: + virObjectListFreeCount(*vms, *nvms); + *vms = NULL; + *nvms = 0; + + return -1; +} + + +int virDomainObjListExport(virDomainObjListPtr domlist, virConnectPtr conn, virDomainPtr **domains, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ac11a20..06ca82f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3059,6 +3059,15 @@ int virDomainObjListExport(virDomainObjListPtr doms, virDomainPtr **domains, virDomainObjListACLFilter filter, unsigned int flags); +int virDomainObjListConvert(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainPtr *doms, + size_t ndoms, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags, + bool skip_missing); int virDomainDefMaybeAddController(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9a7a944..67a7e21 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -386,6 +386,7 @@ virDomainObjGetPersistentDef; virDomainObjGetState; virDomainObjListAdd; virDomainObjListCollect; +virDomainObjListConvert; virDomainObjListExport; virDomainObjListFindByID; virDomainObjListFindByName; -- 2.3.5
On 30.04.2015 14:44, Peter Krempa wrote:
Add virDomainObjListConvert that will take a list of virDomains, apply filters and return a list of virDomainObjs. --- src/conf/domain_conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 64 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66fe470..73dc33f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23072,6 +23072,60 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
int +virDomainObjListConvert(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainPtr *doms, + size_t ndoms, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags, + bool skip_missing) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDomainObjPtr vm; + size_t i; + + *nvms = 0; + *vms = NULL; + + virObjectLock(domlist); + for (i = 0; i < ndoms; i++) { + virDomainPtr dom = doms[i]; + + virUUIDFormat(dom->uuid, uuidstr); + + if (!(vm = virHashLookup(domlist->objs, uuidstr))) { + if (!skip_missing) { + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->name); + virObjectUnlock(domlist);
Move Unlock() before the virReportError(). Not that it would matter, but: a) it keeps list locked for shorter time b) I always felt like ReportError() and goto error should be joined together.
+ goto error; + } + } else { + virObjectRef(vm); + + if (VIR_APPEND_ELEMENT(*vms, *nvms, vm) < 0)
Please unlock the @domlist before jumping anywhere.
+ goto error; + } + } + virObjectUnlock(domlist); + + virDomainObjListFilter(vms, nvms, conn, filter, flags); + + return 0; + + error: + virObjectListFreeCount(*vms, *nvms); + *vms = NULL; + *nvms = 0; + + return -1; +} + +
Michal
On Mon, May 04, 2015 at 13:22:17 +0200, Michal Privoznik wrote:
On 30.04.2015 14:44, Peter Krempa wrote:
Add virDomainObjListConvert that will take a list of virDomains, apply filters and return a list of virDomainObjs. --- src/conf/domain_conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 64 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66fe470..73dc33f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23072,6 +23072,60 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
int +virDomainObjListConvert(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainPtr *doms, + size_t ndoms, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags, + bool skip_missing) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDomainObjPtr vm; + size_t i; + + *nvms = 0; + *vms = NULL; + + virObjectLock(domlist); + for (i = 0; i < ndoms; i++) { + virDomainPtr dom = doms[i]; + + virUUIDFormat(dom->uuid, uuidstr); + + if (!(vm = virHashLookup(domlist->objs, uuidstr))) { + if (!skip_missing) { + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->name); + virObjectUnlock(domlist);
Move Unlock() before the virReportError(). Not that it would matter, but: a) it keeps list locked for shorter time b) I always felt like ReportError() and goto error should be joined together.
Okay.
+ goto error; + } + } else { + virObjectRef(vm); + + if (VIR_APPEND_ELEMENT(*vms, *nvms, vm) < 0)
Please unlock the @domlist before jumping anywhere.
Uhh, right. Also @vm needs to be unref'd before jumping to error.
+ goto error; + } + } + virObjectUnlock(domlist); + + virDomainObjListFilter(vms, nvms, conn, filter, flags); + + return 0; + + error: + virObjectListFreeCount(*vms, *nvms); + *vms = NULL; + *nvms = 0; + + return -1; +} + +
I've also found the function hard to read after a week of hollidays so I've changed the control flow a bit so that the else section is not necessary and I've inverted the (!skip_missing) check to 'continue' the for loop. Thanks for the review I'll push this shortly. Peter
On 04/30/2015 08:44 AM, Peter Krempa wrote:
Add virDomainObjListConvert that will take a list of virDomains, apply filters and return a list of virDomainObjs. --- src/conf/domain_conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 64 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66fe470..73dc33f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23072,6 +23072,60 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
int +virDomainObjListConvert(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainPtr *doms, + size_t ndoms, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags, + bool skip_missing) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDomainObjPtr vm; + size_t i; + + *nvms = 0; + *vms = NULL; + + virObjectLock(domlist); + for (i = 0; i < ndoms; i++) { + virDomainPtr dom = doms[i]; + + virUUIDFormat(dom->uuid, uuidstr); + + if (!(vm = virHashLookup(domlist->objs, uuidstr))) { + if (!skip_missing) { + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->name); + virObjectUnlock(domlist); + goto error; + } + } else { + virObjectRef(vm); + + if (VIR_APPEND_ELEMENT(*vms, *nvms, vm) < 0) + goto error; + } + } + virObjectUnlock(domlist); + + virDomainObjListFilter(vms, nvms, conn, filter, flags);
Coverity complains here too - if 'vms' is NULL, then ObjListFilter derefs. Coverity shows an example of ndoms == 2 and for each iteration through the loop it takes the skip_missing path. John
+ + return 0; + + error: + virObjectListFreeCount(*vms, *nvms); + *vms = NULL; + *nvms = 0; + + return -1; +} + + +int virDomainObjListExport(virDomainObjListPtr domlist, virConnectPtr conn, virDomainPtr **domains, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ac11a20..06ca82f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3059,6 +3059,15 @@ int virDomainObjListExport(virDomainObjListPtr doms, virDomainPtr **domains, virDomainObjListACLFilter filter, unsigned int flags); +int virDomainObjListConvert(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainPtr *doms, + size_t ndoms, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags, + bool skip_missing);
int virDomainDefMaybeAddController(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9a7a944..67a7e21 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -386,6 +386,7 @@ virDomainObjGetPersistentDef; virDomainObjGetState; virDomainObjListAdd; virDomainObjListCollect; +virDomainObjListConvert; virDomainObjListExport; virDomainObjListFindByID; virDomainObjListFindByName;
On Mon, May 11, 2015 at 06:33:00 -0400, John Ferlan wrote:
On 04/30/2015 08:44 AM, Peter Krempa wrote:
Add virDomainObjListConvert that will take a list of virDomains, apply filters and return a list of virDomainObjs. --- src/conf/domain_conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 9 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 64 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 66fe470..73dc33f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23072,6 +23072,60 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
int +virDomainObjListConvert(virDomainObjListPtr domlist, + virConnectPtr conn, + virDomainPtr *doms, + size_t ndoms, + virDomainObjPtr **vms, + size_t *nvms, + virDomainObjListACLFilter filter, + unsigned int flags, + bool skip_missing) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virDomainObjPtr vm; + size_t i; + + *nvms = 0; + *vms = NULL; + + virObjectLock(domlist); + for (i = 0; i < ndoms; i++) { + virDomainPtr dom = doms[i]; + + virUUIDFormat(dom->uuid, uuidstr); + + if (!(vm = virHashLookup(domlist->objs, uuidstr))) { + if (!skip_missing) { + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, dom->name); + virObjectUnlock(domlist); + goto error; + } + } else { + virObjectRef(vm); + + if (VIR_APPEND_ELEMENT(*vms, *nvms, vm) < 0) + goto error; + } + } + virObjectUnlock(domlist); + + virDomainObjListFilter(vms, nvms, conn, filter, flags);
Coverity complains here too - if 'vms' is NULL, then ObjListFilter derefs. Coverity shows an example of ndoms == 2 and for each iteration through the loop it takes the skip_missing path.
False positive again. If every domain is skipped, then nvms will never be incremented from it's 0 state where it was pre-set. virDomainObjListFilter then iterates from 0 while it's less than the value stored in ndoms ( which is 0) thus it never touches @vms. Peter
Use the new domain list collection helpers to avoid going through virDomainPtrs. This additionally implements filter capability when called through the api that accepts domain list filters. --- src/qemu/qemu_driver.c | 86 ++++++++++++++++++++++---------------------------- 1 file changed, 37 insertions(+), 49 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3032b7a..c4a989c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19815,26 +19815,25 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; - virDomainPtr *domlist = NULL; - virDomainObjPtr dom = NULL; + virDomainObjPtr *vms = NULL; + virDomainObjPtr vm; + size_t nvms; virDomainStatsRecordPtr *tmpstats = NULL; bool enforce = !!(flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS); - int ntempdoms; int nstats = 0; size_t i; int ret = -1; unsigned int privflags = 0; unsigned int domflags = 0; + unsigned int lflags = flags & (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE); - if (ndoms) - virCheckFlags(VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING | - VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1); - else - virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE | - VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT | - VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE | - VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING | - VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1); + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE | + VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT | + VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE | + VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING | + VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1); if (virConnectGetAllDomainStatsEnsureACL(conn) < 0) return -1; @@ -19842,58 +19841,53 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (qemuDomainGetStatsCheckSupport(&stats, enforce) < 0) return -1; - if (!ndoms) { - unsigned int lflags = flags & (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE | - VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT | - VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE); - - if ((ntempdoms = virDomainObjListExport(driver->domains, - conn, - &domlist, - virConnectGetAllDomainStatsCheckACL, - lflags)) < 0) - goto cleanup; - - ndoms = ntempdoms; - doms = domlist; + if (ndoms) { + if (virDomainObjListConvert(driver->domains, conn, doms, ndoms, &vms, + &nvms, virConnectGetAllDomainStatsCheckACL, + lflags, true) < 0) + return -1; + } else { + if (virDomainObjListCollect(driver->domains, conn, &vms, &nvms, + virConnectGetAllDomainStatsCheckACL, + lflags) < 0) + return -1; } - if (VIR_ALLOC_N(tmpstats, ndoms + 1) < 0) - goto cleanup; + if (VIR_ALLOC_N(tmpstats, nvms + 1) < 0) + return -1; if (qemuDomainGetStatsNeedMonitor(stats)) privflags |= QEMU_DOMAIN_STATS_HAVE_JOB; - for (i = 0; i < ndoms; i++) { + for (i = 0; i < nvms; i++) { virDomainStatsRecordPtr tmp = NULL; domflags = 0; + vm = vms[i]; - if (!(dom = qemuDomObjFromDomain(doms[i]))) - continue; - - if (doms != domlist && - !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) { - virDomainObjEndAPI(&dom); - continue; - } + virObjectLock(vm); if (HAVE_JOB(privflags) && - qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) == 0) + qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) == 0) domflags |= QEMU_DOMAIN_STATS_HAVE_JOB; /* else: without a job it's still possible to gather some data */ if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING) domflags |= QEMU_DOMAIN_STATS_BACKING; - if (qemuDomainGetStats(conn, dom, stats, &tmp, domflags) < 0) - goto endjob; + if (qemuDomainGetStats(conn, vm, stats, &tmp, domflags) < 0) { + if (HAVE_JOB(domflags) && vm) + qemuDomainObjEndJob(driver, vm); + + virObjectUnlock(vm); + goto cleanup; + } if (tmp) tmpstats[nstats++] = tmp; if (HAVE_JOB(domflags)) - qemuDomainObjEndJob(driver, dom); + qemuDomainObjEndJob(driver, vm); - virDomainObjEndAPI(&dom); + virObjectUnlock(vm); } *retStats = tmpstats; @@ -19901,15 +19895,9 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, ret = nstats; - endjob: - if (HAVE_JOB(domflags) && dom) - qemuDomainObjEndJob(driver, dom); - cleanup: - virDomainObjEndAPI(&dom); - virDomainStatsRecordListFree(tmpstats); - virObjectListFree(domlist); + virObjectListFreeCount(vms, nvms); return ret; } -- 2.3.5
On 30.04.2015 14:44, Peter Krempa wrote:
Peter Krempa (6): util: Make the virDomainListFree helper more universal conf: Extract code to filter domain list into a separate function conf: Rename virDomainObjListFilter type to virDomainObjListACLFilter conf: Refactor domain list collection critical section conf: Add helper to convert list of virDomains to a list of virDomainObjs qemu: Convert qemuConnectGetAllDomainStats to use new helpers
daemon/remote.c | 2 +- src/conf/domain_conf.c | 258 ++++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 29 +++-- src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 86 +++++++-------- src/util/virobject.c | 41 +++++++ src/util/virobject.h | 2 + tools/virsh-domain-monitor.c | 2 +- 8 files changed, 275 insertions(+), 150 deletions(-)
ACK series, but see my comment to 5/6 before pushing. Michal
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa