On 06/05/2012 07:19 AM, Peter Krempa wrote:
> This patch adds common code to list domains in fashion used by
> virListAllDomains and helpers to filter this list.
>
> This code supports filters based on data stored in the virDomainObj.
> Supported filter flags:
> VIR_CONNECT_LIST_DOMAINS_ACTIVE
> VIR_CONNECT_LIST_DOMAINS_INACTIVE
> VIR_CONNECT_LIST_DOMAINS_PERSISTENT
> VIR_CONNECT_LIST_DOMAINS_TRANSIENT
> VIR_CONNECT_LIST_DOMAINS_RUNNING
> VIR_CONNECT_LIST_DOMAINS_PAUSED
> VIR_CONNECT_LIST_DOMAINS_SHUTOFF
> VIR_CONNECT_LIST_DOMAINS_OTHER
> VIR_CONNECT_LIST_DOMAINS_AUTOSTART
> VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART
> VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT
> VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT
>
> Hypervisor specific flags have to be filtered after using this function.
> This patch adds virDomainListFilter() helper function to remove elements
> from the array based on a predicate.
You know, the only flag group you don't have here is
DOMAINS_[NO_]MANAGEDSAVE, but it wouldn't be that hard to update
virDomainObjPtr to have a new bool field that tracks this information
(update the information when a managedsave image is created, deleted,
and when reloading domain state at libvirtd startup). Besides, we
already have precedence for that - it is how we manage domain snapshots
(that is, qemu_driver.c updates vm->snapshots at key points in snapshot
life cycle).
I'd rather see a pre-requisite patch that adds a new 'has_managedsave'
bool flag to the struct, at which point [0]...
> ---
> New in this series.
> ---
> src/Makefile.am | 6 +-
> src/conf/virdomainlist.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/virdomainlist.h | 36 ++++++++
> src/libvirt_private.syms | 5 +
> 4 files changed, 260 insertions(+), 1 deletions(-)
> create mode 100644 src/conf/virdomainlist.c
> create mode 100644 src/conf/virdomainlist.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 5693fb4..fd9d892 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -194,6 +194,9 @@ CPU_CONF_SOURCES = \
> CONSOLE_CONF_SOURCES = \
> conf/virconsole.c conf/virconsole.h
>
> +# Domain listing helpers
> +DOMAIN_LIST_SOURCES = \
> + conf/virdomainlist.c conf/virdomainlist.h
> CONF_SOURCES = \
While what you have works, I like to add a blank line between any macro
definition that uses \-newline to take up more than one source line, so
that it is a bit more obvious that the last line should not have \ (or
if someone does accidentally put \ on the last line, at least the next
macro name is still an independent macro name rather than an unintended
continuation of the first macro). That said...
> $(NETDEV_CONF_SOURCES) \
> $(DOMAIN_CONF_SOURCES) \
> @@ -206,7 +209,8 @@ CONF_SOURCES = \
> $(INTERFACE_CONF_SOURCES) \
> $(SECRET_CONF_SOURCES) \
> $(CPU_CONF_SOURCES) \
> - $(CONSOLE_CONF_SOURCES)
> + $(CONSOLE_CONF_SOURCES) \
> + $(DOMAIN_LIST_SOURCES)
...I would just inline the listing of the two new files to
DOMAIN_CONF_SOURCES rather than creating a new category DOMAIN_LIST_SOURCES.
>
> # The remote RPC driver, covering domains, storage, networks, etc
> REMOTE_DRIVER_GENERATED = \
> diff --git a/src/conf/virdomainlist.c b/src/conf/virdomainlist.c
> new file mode 100644
> index 0000000..180b37d
> --- /dev/null
> +++ b/src/conf/virdomainlist.c
> @@ -0,0 +1,214 @@
> +/**
> + * virdomainlist.c: Helpers for listing and filtering domains.
> + *
> + * Copyright (C) 2011-2012 Red Hat, Inc.
Are we really borrowing any contents written in 2011, or can this be
shortened to just 2012?
> +#define VIR_FROM_THIS VIR_FROM_NONE
s/VIR_FROM_NONE/VIR_FROM_DOMAIN/
> +
> +struct virDomainListData {
> + virConnectPtr conn;
> + virDomainPtr *domains;
> + unsigned int flags;
> + int ndomains;
> + size_t size;
I would consider adding a second size_t variable, so that you can track
usage independently from allocations. In other words, the number of
domains can be large enough that reallocating the array for every member
can be noticeably slower than reallocating the array geometrically [1].
Or, don't even bother with reallocations [2].
> + /* filter by domain state */
> + if (MATCH(VIR_CONNECT_LIST_DOMAINS_RUNNING |
> + VIR_CONNECT_LIST_DOMAINS_PAUSED |
> + VIR_CONNECT_LIST_DOMAINS_SHUTOFF |
> + VIR_CONNECT_LIST_DOMAINS_OTHER)) {
> + int st = virDomainObjGetState(vm, NULL);
> + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_RUNNING) &&
> + st == VIR_DOMAIN_RUNNING) ||
> + (MATCH(VIR_CONNECT_LIST_DOMAINS_PAUSED) &&
> + st == VIR_DOMAIN_PAUSED) ||
> + (MATCH(VIR_CONNECT_LIST_DOMAINS_SHUTOFF) &&
> + st == VIR_DOMAIN_SHUTOFF) ||
> + (MATCH(VIR_CONNECT_LIST_DOMAINS_OTHER) &&
> + (st == VIR_DOMAIN_NOSTATE || st == VIR_DOMAIN_BLOCKED ||
> + st == VIR_DOMAIN_SHUTDOWN || st == VIR_DOMAIN_CRASHED ||
> + st == VIR_DOMAIN_PMSUSPENDED))))
Rather than listing all the other states here (and missing something
when a new state is added), I would write this clause as:
(MATCH(VIR_CONNECT_LIST_DOMAINS_OTHER) &&
st != VIR_DOMAIN_RUNNING &&
st != VIR_DOMAIN_PAUSED &&
st != VIR_DOMAIN_SHUTOFF)
> +
> + /* filter by snapshot existence */
> + if (MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT |
> + VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT)) {
> + int nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0);
> + if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT) &&
> + nsnapshots > 0) ||
> + (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT) &&
> + nsnapshots <= 0)))
> + goto cleanup;
> + }
Careful. ESX and VBox support snapshots, but do not track them in
&vm->snapshots. We can keep this common filtering only if those two
hypervisors mask out both HAS_SNAPSHOT and NO_SNAPSHOT bits before
calling into this function. Then again, those two hypervisors don't use
virDomainObj in the first place, so maybe I'm getting ahead of myself.
> +
> + /* add the domain to the result list */
> + if (VIR_EXPAND_N(data->domains, data->size, 1) < 0)
[1] Here, I'd use the four-argument VIR_RESIZE_N, for geometric growth
of the array, if we even keep the realloc as part of this callback.
> +int
> +virDomainList(virConnectPtr conn, virHashTablePtr domobjs,
> + virDomainPtr **domains, unsigned int flags)
> +{
> + int ret = -1;
> + int i;
> +
> + struct virDomainListData data = { conn, NULL, flags, 0, 0, false };
Why the double space before NULL?
> +
> + if (domains) {
> + if (VIR_ALLOC_N(data.domains, 1) < 0) {
[2] Why not just allocate the end result according to the size of the
hash table? We know that the result cannot exceed virHashSize(domobjs)
(it might be smaller, but if we overallocate now, then we don't have to
mess with realloc in the middle of each callback).
> +
> +int
> +virDomainListFilter(virDomainPtr **doms,
> + int *ndoms,
> + virDomainListFilterFunc filter,
> + int result)
> +{
[0]... we might not even need this function, if you can get the common
virDomainObjPtr code to track managedsave images in place (at least,
that was the only reason that 5/9 used it; I didn't check if 6/9 or 7/9
needed to use it for other reasons).
> + int i = 0;
> + int ret;
> +
> + while (i < *ndoms) {
> + ret = filter((*doms)[i]);
> + if (ret < 0)
> + return -1;
> +
> + if (ret == result) {
> + virDomainFree((*doms)[i]);
> + if (i != --*ndoms)
> + memmove(&(*doms)[i], &(*doms)[i+1], sizeof(*doms) * (*ndoms - i));
> +
> + if (VIR_REALLOC_N(*doms, *ndoms) < 0) {
VIR_SHRINK_N might be nicer here.
> diff --git a/src/conf/virdomainlist.h b/src/conf/virdomainlist.h
> new file mode 100644
> index 0000000..80fe4f8
> --- /dev/null
> +++ b/src/conf/virdomainlist.h
> @@ -0,0 +1,36 @@
> +/**
> + * virdomainlist.h: Helpers for listing and filtering domains.
> + *
> + * Copyright (C) 2011-2012 Red Hat, Inc.
Same question on copyright year.
Worth another round of review on this patch.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list