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@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