On 05/20/2012 09:56 AM, Peter Krempa wrote:
> This patch adds a new public api that lists domains. The new approach is
> different from those used before. There are four key points to this:
>
> 1) The list is acquired atomicaly and contains both active and inactive
s/atomicaly/atomically/
> domains (guests). This eliminates the need to call two different list
> APIs, where the state might change in between of the calls.
Here, you may want to merge in points from my RFC in your v2, such as
the ability to do filtering.
>
> 2) The returned list consists of virDomainPtrs instead of names or ID's
> that have to be converted to virDomainPtrs anyways using separate calls
> for each one of them. This is more convenient and saves resources and
> application code in most (all?) cases.
I don't know about 'saves resources' - we are actually passing more over
the RPC wire. But definitely more convenient, and less application code.
>
> 3) The returned list is auto-allocated. This saves a lot hassle for the
> users.
>
> 4) The python binding returns a list of domain objects that is very neat
> to work with.
>
> The only problem with this approach is no support from code generators
> so both RPC code and python bindings had to be written manualy.
s/manualy/manually/
>
> *include/libvirt/libvirt.h.in: - add API prototype
> - clean up whitespace mistakes nearby
> *python/generator.py: - inhibit generation of the bindings for the new
> api
> *src/driver.h: - add driver prototype
> - clean up some whitespace mistakes nearby
> *src/libvirt.c: - add public implementation
> *src/libvirt_public.syms: - export the new symbol
> ---
> include/libvirt/libvirt.h.in | 8 +++++-
> python/generator.py | 1 +
> src/driver.h | 12 ++++++++--
> src/libvirt.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> src/libvirt_public.syms | 5 ++++
> 5 files changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index a817db8..2dacd45 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1759,8 +1759,12 @@ int virDomainUndefineFlags (virDomainPtr domain,
> unsigned int flags);
> int virConnectNumOfDefinedDomains (virConnectPtr conn);
> int virConnectListDefinedDomains (virConnectPtr conn,
> - char **const names,
> - int maxnames);
> + char **const names,
> + int maxnames);
> +int virConnectListAllDomains (virConnectPtr conn,
> + virDomainPtr **domains,
Hmm - time for me to think out loud. Your signature differs from mine.
I proposed:
virDomainPtr *domains
aka
virDomain **domains
where my return value would be an array of virDomain objects, and the
caller would have to do:
virDomainPtr dom, first;
virConnectListAllDomains(conn, &first, flags);
dom = first;
while (dom) {
use dom;
virDomainFree(dom);
dom++;
}
free(first);
But that would only work if virDomain is not an opaque type.
You proposed:
virDomainPtr **domains
aka
virDomain ***domains
where the return is an array of virDomainPtr pointers. The caller would
then do:
virDomainPtr *array;
virDomainPtr dom;
int i = 0;
virConnectListAllDomains(conn, &array, flags);
dom = array[0];
do {
use dom;
virDomainFree(dom);
dom = array[i++];
} while (dom);
free(array);
(Of course, you could use a for (i=0; i < ret; i++) loop instead of a
while loop; I chose that style to constrast the difference between the
number of pointer dereferences needed). In conclusion, I think your
style is right. But it also means that we ought to document a sample
usage as part of the API call :)
> + int ndomains,
Drop the ndomains argument.
> + unsigned int flags);
> int virDomainCreate (virDomainPtr domain);
> int virDomainCreateWithFlags (virDomainPtr domain,
> unsigned int flags);
> diff --git a/python/generator.py b/python/generator.py
> index 9530867..a81fe4d 100755
> --- a/python/generator.py
> +++ b/python/generator.py
> @@ -453,6 +453,7 @@ skip_function = (
> 'virConnectDomainEventDeregisterAny', # overridden in virConnect.py
> 'virSaveLastError', # We have our own python error wrapper
> 'virFreeError', # Only needed if we use virSaveLastError
> + 'virConnectListAllDomains', #overriden in virConnect.py
s/overriden/overridden/
> +++ b/src/driver.h
> @@ -251,9 +251,14 @@ typedef char *
> const char *domainXml,
> unsigned int flags);
> typedef int
> - (*virDrvListDefinedDomains) (virConnectPtr conn,
> - char **const names,
> - int maxnames);
> + (*virDrvListDefinedDomains) (virConnectPtr conn,
> + char **const names,
> + int maxnames);
> +typedef int
> + (*virDrvConnectListAllDomains) (virConnectPtr conn,
> + virDomainPtr **domains,
> + int ndomains,
Again, drop ndomains.
> @@ -1014,6 +1019,7 @@ struct _virDriver {
> virDrvDomainGetDiskErrors domainGetDiskErrors;
> virDrvDomainSetMetadata domainSetMetadata;
> virDrvDomainGetMetadata domainGetMetadata;
> + virDrvConnectListAllDomains connectListAllDomains;
Order doesn't matter in this file; so I'd stick it closer to
listDomains/numOfDomains, not here at the end. Your name is long; my
preliminary code went with the shorter 'listAllDomains'.
>
> /**
> + * virConnectListAllDomains:
> + * @conn: Pointer to the hypervisor connection.
> + * @domains: Pointer to a variable to store the array containing domain objects
> + * or NULL if the list is not required (just returns number of guests).
Nice idea - I hadn't thought of allowing NULL for counting.
> + * @ndomains: Maximum number of requested guests. Set to -1 for no limits.
Drop.
> + * @flags: Not used yet. Callers must pass 0.
See my RFC on how to use this for filtering.
> + *
> + * List all guests (domains). This function allocates and returns a complete list
> + * of guests. Caller has to free the returned virDomainPtrs and the array.
My wording mentioned: virDomainFree() on each guest, then free() on the
array. Be specific, otherwise someone will do it wrong and leak memory.
Again, sample code in this API can't hurt.
> + *
> + * Returns number of guests found (and stored in *domains) or -1 on error.
> + */
> +int
> +virConnectListAllDomains(virConnectPtr conn,
> + virDomainPtr **domains,
> + int ndomains,
> + unsigned int flags)
> +{
> + VIR_DEBUG("conn=%p, domains=%p, ndomains=%d, flags=%x",
> + conn, domains, ndomains, flags);
Adjust for the loss of ndomains.
--
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