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

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