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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org