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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list