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...
+ 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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)