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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list