On 05/04/2011 08:45 AM, Jiri Denemark wrote:
> Reason is currently always set to 0 (i.e., *_UNKNOWN).
> ---

> +++ b/src/xen/xen_driver.c
> @@ -1010,6 +1010,22 @@ xenUnifiedDomainGetInfo (virDomainPtr dom, 
> virDomainInfoPtr info)
>  }
>  
>  static int
> +xenUnifiedDomainGetState(virDomainPtr dom, int *state, int *reason)
> +{
> +    GET_PRIVATE(dom->conn);
> +    int i;
> +
> +    for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) {
> +        if (priv->opened[i] &&
> +            drivers[i]->domainGetState &&
> +            drivers[i]->domainGetState(dom, state, reason) == 0)
> +            return 0;
> +    }

...
> +int
> +xenHypervisorGetDomainState(virDomainPtr domain, int *state, int *reason)

You definitely need this function in xen_hypervisor.c, but I still
wonder if the for loop across callbacks in xen_driver.c is appropriate,
given that we are trying to get rid of the secondary callback structure,
or if we should just inline direct calls to the appropriate helper (I
brought up this same point in my review of 2/8).  After all, not all
possible xen helpers implement the callback (for example, xen_inotify.c
does not).

> +++ b/src/xen/xen_hypervisor.h
> @@ -66,6 +66,10 @@ int     xenHypervisorPauseDomain        (virDomainPtr 
> domain)
>  int     xenHypervisorGetDomainInfo        (virDomainPtr domain,
>                                             virDomainInfoPtr info)
>            ATTRIBUTE_NONNULL (1);
> +int     xenHypervisorGetDomainState     (virDomainPtr domain,
> +                                         int *state,
> +                                         int *reason)
> +          ATTRIBUTE_NONNULL (1);

That is, either we use the callback structure, and this function could
be private, or we declare this function and call it directly, then we
don't need an extra row in the secondary callback structure.

> +++ b/src/xen/xs_internal.h
> @@ -23,6 +23,9 @@ virDrvOpenStatus    xenStoreOpen    (virConnectPtr conn,
>  int          xenStoreClose           (virConnectPtr conn);
>  int          xenStoreGetDomainInfo   (virDomainPtr domain,
>                                           virDomainInfoPtr info);
> +int             xenStoreDomainGetState  (virDomainPtr domain,

Spacing looks awkward here (space vs. tab).

You'll have tweaks to fold in a flag parameter, but overall this patch
looked sane.

ACK.

-- 
Eric Blake   ebl...@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to