Re: [libvirt] [PATCHv1 7/9] hyperv: Add implementation for virConnectListAllDomains()
On 06/05/2012 07:19 AM, Peter Krempa wrote:
Hyperv doesn't use the common virDomainObjimplementation so
this patch adds a separate implementation.
This driver implementation supports only the following filter flags:
VIR_CONNECT_LIST_DOMAINS_ACTIVE
VIR_CONNECT_LIST_DOMAINS_INACTIVE
VIR_CONNECT_LIST_DOMAINS_TRANSIENT
VIR_CONNECT_LIST_DOMAINS_PERSISTENT
The latter two of these are irelevant as Hyperv only supports persistent
s/irelevant/irrelevant/
domains, so specifying only VIR_CONNECT_LIST_DOMAINS_TRANSIENT
results
into an empty list.
It looks like hypervDomainGetInfo uses a helper
hypervMsvmComputerSystemEnabledStateToDomainState to get domain state,
so you should also be able to support _RUNNING, _PAUSED, _SHUTOFF, and
_OTHER - and I'd really like to guarantee that much for all domains that
implement this API. Also, since managedsave is present
(hypervDomainHasManagedSaveImage), you should definitely support
[NO]_MANAGEDSAVE. You could also trivially support [HAS|NO]_SNAPSHOT in
the same way that you trivially support _PERSISTENT by always returning
0 for that one, but the API documentation will let us go either way.
---
New in series. UNTESTED!! (I don't have access to Hyperv, and couldn't even get
dependencies to compile this driver, so I'm not even sure if this compiles.)
I don't have access to test hyperv, but I do have the dependencies for
compilation (but don't ask me to remember how I got it installed), and
can report:
Compilation succeeded!
+static int
+hypervListAllDomains(virConnectPtr conn,
+ virDomainPtr **domains,
+ unsigned int flags)
+{
+ hypervPrivate *priv = conn->privateData;
+ virBuffer query = VIR_BUFFER_INITIALIZER;
+ Msvm_ComputerSystem *computerSystemList = NULL;
+ Msvm_ComputerSystem *computerSystem = NULL;
+ size_t ndoms;
+ virDomainPtr domain;
+ virDomainPtr *doms = NULL;
+ int count = 0;
+ int ret = -1;
+ int i;
Based on comparison with other code, what you have seems reasonable, but
incomplete given what else I think is possible by copy-and-paste.
+ if (domains) {
+ if (VIR_ALLOC_N(doms, 1) < 0)
+ goto no_memory;
+ ndoms = 1;
+ }
+
+ for (computerSystem = computerSystemList; computerSystem != NULL;
+ computerSystem = computerSystem->next) {
+
+ if (!doms) {
+ count++;
+ continue;
Unlike my complaints in 5/9 and 6/9, here, you really are best doing a
reallocation during the iteration, since you don't know up front how
long the list is. However, I also think that you can use VIR_RESIZE_N
along with an extra allocation tracking variable for geometric growth
and less overhead than quadratic effects from reallocation on every
iteration.
static virDriver hypervDriver = {
.no = VIR_DRV_HYPERV,
@@ -1294,6 +1388,7 @@ static virDriver hypervDriver = {
.domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */
.domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */
.isAlive = hypervIsAlive, /* 0.9.8 */
+ .listAllDomains = hypervListAllDomains, /* 0.9.13 */
As with other patches in the series, I'd consider listing this in the
order where it appears in driver.h, although it is not a show-stopper.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)