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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/