[libvirt] [PATCHv3 08/12] vbox: Add support for virConnectListAllDomains()

[Eric Blake]

On 06/11/2012 04:34 AM, Peter Krempa wrote:
> VirtualBox doesn't use the common virDomainObj implementation so this
> patch adds a separate implementation using the VirtualBox API.
> 
> This driver implementation supports all currently defined flags. As
> VirtualBox does not support transient guests, managed save images and
> autostarting we assume all guests are persistent, don't have a managed
> save image and are not autostarted. Filtering for existence of those
> properities results in empty list.
> ---
> Diff to v2:
> -added support for all filter flags
> -changed allocation of domains array
> ---
>  src/vbox/vbox_tmpl.c |  170 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 170 insertions(+), 0 deletions(-)
> 

Compile-tested, but not runtime tested.  If you'd like a runtime test,
I'd suggest waiting for some feedback from Matthias (cc'd).  Meanwhile,
even by inspection, I found a logic bug, so this needs a tweak:

> +
> +#define MATCH(FLAG) (flags & (FLAG))
> +static int
> +vboxListAllDomains(virConnectPtr conn,
> +                   virDomainPtr **domains,
> +                   unsigned int flags)
> +{

> +    virCheckFlags(VIR_CONNECT_LIST_FILTERS_ALL, -1);
> +
> +    /* filter out flag options that will produce 0 results in vbox driver:
> +     * - managed save: vbox guests don't have managed save images
> +     * - autostart: vbox doesn't support autostarting guests
> +     * - persistance: vbox doesn't support transient guests
> +     */
> +    if ((MATCH(VIR_CONNECT_LIST_DOMAINS_TRANSIENT) &&
> +         !MATCH(VIR_CONNECT_LIST_DOMAINS_PERSISTENT)) ||
> +        (MATCH(VIR_CONNECT_LIST_DOMAINS_AUTOSTART) &&
> +         !MATCH(VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART)) ||
> +        (MATCH(VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE) &&
> +         !MATCH(VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE))) {
> +        if (domains &&
> +            VIR_ALLOC_N(*domains, 1) < 0)

Nice pre-filter, and good that you remembered to still allocate the
trailing NULL.

> +
> +    if (domains &&
> +        VIR_ALLOC_N(doms, machines.count+1) < 0)

Style nit - space around '+'.

> 
> +                if (state >= MachineState_FirstOnline &&
> +                    state <= MachineState_LastOnline)
> +                    active = true;
> +                else
> +                    active = false;

Here, you check active domains, then you filter, ...

> +                if (!dom)
> +                    goto no_memory;
> +
> +                if (active)
> +                    dom->id = i + 1;

...and finally, you compute domain ids.  But if the filter removed a
domain, you forgot to increment your counter.  You need to float the
active domain counting prior to any filtering, even though you only
assign it into dom->id after filtering succeeds.

> +    if (doms) {
> +        /* safe to ignore, new size will be less or equal than
> +         * previous allocation*/
> +        ignore_value(VIR_REALLOC_N(doms, count+1));

Another place for space around '+'.

> +        *domains = doms;
> +    }
> +    doms = NULL;
> +    ret = count;
> +
> +cleanup:
> +    if (doms) {

Technically, count is only ever non-zero if doms was successfully
allocated, in which case you can lose this outer 'if', and...

> +        for (i = 0; i < count; i++) {

...this 'for' will still do the right thing...

> +            if (doms[i])

...at preventing a dereference of uninitialized doms.

> +                virDomainFree(doms[i]);
> +        }
> +    }
> +    VIR_FREE(doms);
> +
> +    vboxArrayRelease(&machines);
> +    return ret;
> +

Nearly there.

-- 
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/20120618/fc6a05cf/attachment-0001.sig>