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

[Peter Krempa]

On 06/18/12 22:06, Eric Blake wrote:
> 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.

i is the iteration variable of the outer for loop, so it's incremented 
automaticaly in the for statement and is unique per domain. This loop is 
actualy stol..borrowed from the legacy listing code.
>
>> +    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...

Well, count is non zero and doms is NULL when you try to count the 
machines so we need to keep this check.

>

Peter