[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 7/8] Implement basic virDomainGetState in all drivers



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   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]