[Libvir] PATCH 13/20: create internal stateful driver API

[Daniel Veillard]

On Fri, Jun 22, 2007 at 03:07:08AM +0100, Daniel P. Berrange wrote:
> Some drivers are stateless (Xen, test), while others are stateful (QEMU).
> The later can only be accessed via the daemon. This patch adds a new internal
> driver API to allow drivers to register a set of functions for performing
> work on daemon startup, shutdown and reload. It makes the QEMU driver in
> qemud/driver.c provide implementations of these funtions. It adapts the
> qemud/qemud.c to call these new global driver functions. Finally it fixes
> the idle timeout shutdown of the daemon disabled in an earlier patch.
> NB this patch adds 3 new exported symbols for private use by the daemon.

[...]
> +typedef int (*virDrvStateInitialize) (void);
> +typedef int (*virDrvStateCleanup) (void);
> +typedef int (*virDrvStateReload) (void);
> +typedef int (*virDrvStateActive) (void);
> +
> +typedef struct _virStateDriver virStateDriver;
> +typedef virStateDriver *virStateDriverPtr;
> +
> +struct _virStateDriver {
> +    virDrvStateInitialize  initialize;
> +    virDrvStateCleanup     cleanup;
> +    virDrvStateReload      reload;
> +    virDrvStateActive      active;
> +};
>  
>  /*
>   * Registration
> @@ -324,6 +338,7 @@ struct _virNetworkDriver {
>   */
>  int virRegisterDriver(virDriverPtr);
>  int virRegisterNetworkDriver(virNetworkDriverPtr);
> +int virRegisterStateDriver(virStateDriverPtr);


  Hum, shouldn't that be more closely associated to the virDriver itself
it looks completely orthogonal, so I'm a bit surprized.

>  #ifdef __cplusplus
>  }
> diff -r 968ca2c71e5f src/internal.h
> --- a/src/internal.h	Thu Jun 21 21:20:57 2007 -0400
> +++ b/src/internal.h	Thu Jun 21 21:20:58 2007 -0400
> @@ -214,6 +214,15 @@ int		virFreeNetwork	(virConnectPtr conn,
>  #define virGetDomain(c,n,u) __virGetDomain((c),(n),(u))
>  #define virGetNetwork(c,n,u) __virGetNetwork((c),(n),(u))
>  
> +int __virStateInitialize(void);
> +int __virStateCleanup(void);
> +int __virStateReload(void);
> +int __virStateActive(void);
> +#define virStateInitialize() __virStateInitialize()
> +#define virStateCleanup() __virStateCleanup()
> +#define virStateReload() __virStateReload()
> +#define virStateActive() __virStateActive()

  Funky, a small comment why we do this might help newbies going
over the code.

  /*
   * those function are exported by the library but not supposed to be
   * used for normal use of libvirt.
   */
 or something similar.

> --- a/src/libvirt.c	Thu Jun 21 21:20:57 2007 -0400
> +++ b/src/libvirt.c	Thu Jun 21 21:20:58 2007 -0400
> @@ -40,6 +40,8 @@ static int virDriverTabCount = 0;
>  static int virDriverTabCount = 0;
>  static virNetworkDriverPtr virNetworkDriverTab[MAX_DRIVERS];
>  static int virNetworkDriverTabCount = 0;
> +static virStateDriverPtr virStateDriverTab[MAX_DRIVERS];
> +static int virStateDriverTabCount = 0;
>  static int initialized = 0;

  I'm still a bit surprized this is separate from the drivers,
somehow I would have expected the virStateDriverPtr to be a subfield
of virDriver

> diff -r 968ca2c71e5f src/libvirt_sym.version
> --- a/src/libvirt_sym.version	Thu Jun 21 21:20:57 2007 -0400
> +++ b/src/libvirt_sym.version	Thu Jun 21 21:20:58 2007 -0400
> @@ -101,5 +101,10 @@
>  
>  	__virEventRegisterImpl;
>  
> + 	__virStateInitialize;
> + 	__virStateCleanup;
> + 	__virStateReload;
> + 	__virStateActive;
> +

  Are all the __functions used by the daemon ? making a quick check in the
end and adding a comment would make sense.

  But I think the patch is fine even if I'm a bit surprised about the way
state is handled. I would have though it was a per driver property and hence
associated to the driver structure.

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/