[libvirt] [PATCHv1 7/9] hyperv: Add implementation for virConnectListAllDomains()

[Eric Blake]

On 06/05/2012 07:19 AM, Peter Krempa wrote:
> Hyperv doesn't use the common virDomainObjimplementation so
> this patch adds a separate implementation.
> 
> This driver implementation supports only the following filter flags:
> VIR_CONNECT_LIST_DOMAINS_ACTIVE
> VIR_CONNECT_LIST_DOMAINS_INACTIVE
> VIR_CONNECT_LIST_DOMAINS_TRANSIENT
> VIR_CONNECT_LIST_DOMAINS_PERSISTENT
> The latter two of these are irelevant as Hyperv only supports persistent

s/irelevant/irrelevant/

> domains, so specifying only VIR_CONNECT_LIST_DOMAINS_TRANSIENT results
> into an empty list.

It looks like hypervDomainGetInfo uses a helper
hypervMsvmComputerSystemEnabledStateToDomainState to get domain state,
so you should also be able to support _RUNNING, _PAUSED, _SHUTOFF, and
_OTHER - and I'd really like to guarantee that much for all domains that
implement this API.  Also, since managedsave is present
(hypervDomainHasManagedSaveImage), you should definitely support
[NO]_MANAGEDSAVE.  You could also trivially support [HAS|NO]_SNAPSHOT in
the same way that you trivially support _PERSISTENT by always returning
0 for that one, but the API documentation will let us go either way.

> ---
> New in series. UNTESTED!! (I don't have access to Hyperv, and couldn't even get dependencies to compile this driver, so I'm not even sure if this compiles.)

I don't have access to test hyperv, but I do have the dependencies for
compilation (but don't ask me to remember how I got it installed), and
can report:

Compilation succeeded!

> 
> +static int
> +hypervListAllDomains(virConnectPtr conn,
> +                     virDomainPtr **domains,
> +                     unsigned int flags)
> +{
> +    hypervPrivate *priv = conn->privateData;
> +    virBuffer query = VIR_BUFFER_INITIALIZER;
> +    Msvm_ComputerSystem *computerSystemList = NULL;
> +    Msvm_ComputerSystem *computerSystem = NULL;
> +    size_t ndoms;
> +    virDomainPtr domain;
> +    virDomainPtr *doms = NULL;
> +    int count = 0;
> +    int ret = -1;
> +    int i;

Based on comparison with other code, what you have seems reasonable, but
incomplete given what else I think is possible by copy-and-paste.


> +    if (domains) {
> +        if (VIR_ALLOC_N(doms, 1) < 0)
> +            goto no_memory;
> +        ndoms = 1;
> +    }
> +
> +    for (computerSystem = computerSystemList; computerSystem != NULL;
> +         computerSystem = computerSystem->next) {
> +
> +        if (!doms) {
> +            count++;
> +            continue;

Unlike my complaints in 5/9 and 6/9, here, you really are best doing a
reallocation during the iteration, since you don't know up front how
long the list is.  However, I also think that you can use VIR_RESIZE_N
along with an extra allocation tracking variable for geometric growth
and less overhead than quadratic effects from reallocation on every
iteration.


>  static virDriver hypervDriver = {
>      .no = VIR_DRV_HYPERV,
> @@ -1294,6 +1388,7 @@ static virDriver hypervDriver = {
>      .domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */
>      .domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */
>      .isAlive = hypervIsAlive, /* 0.9.8 */
> +    .listAllDomains = hypervListAllDomains, /* 0.9.13 */

As with other patches in the series, I'd consider listing this in the
order where it appears in driver.h, although it is not a show-stopper.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120606/95c782fd/attachment-0001.sig>