Re: [libvirt] [PATCH 1/2] virDomainEvent refactoring to separate generic things from domain ones
On Tue, Nov 19, 2013 at 04:43:13PM +0100, Cédric Bosdonnat wrote:
The idea behind this commit is to make use of the Domain event
mechanism for other events like network ones. This introduces some
renaming from virDomainEvent* to virOjbectEvent*.
[snip]
-struct _virDomainMeta {
- int id;
- char *name;
- unsigned char uuid[VIR_UUID_BUFLEN];
-};
-typedef struct _virDomainMeta virDomainMeta;
-typedef virDomainMeta *virDomainMetaPtr;
-
-struct _virDomainEventCallbackList {
- unsigned int nextID;
- unsigned int count;
- virDomainEventCallbackPtr *callbacks;
-};
-
-struct _virDomainEventQueue {
- unsigned int count;
- virDomainEventPtr *events;
-};
-
-struct _virDomainEventState {
- /* The list of domain event callbacks */
- virDomainEventCallbackListPtr callbacks;
- /* The queue of domain events */
- virDomainEventQueuePtr queue;
- /* Timer for flushing events queue */
- int timer;
- /* Flag if we're in process of dispatching */
- bool isDispatching;
- virMutex lock;
-};
-
-struct _virDomainEventCallback {
- int callbackID;
- int eventID;
- virConnectPtr conn;
- virDomainMetaPtr dom;
- virConnectDomainEventGenericCallback cb;
- void *opaque;
- virFreeCallback freecb;
- int deleted;
-};
-
-struct _virDomainEvent {
- int eventID;
-
- virDomainMeta dom;
-
- union {
- struct {
- int type;
- int detail;
- } lifecycle;
- struct {
- long long offset;
- } rtcChange;
- struct {
- int action;
- } watchdog;
- struct {
- char *srcPath;
- char *devAlias;
- int action;
- char *reason;
- } ioError;
- struct {
- int phase;
- virDomainEventGraphicsAddressPtr local;
- virDomainEventGraphicsAddressPtr remote;
- char *authScheme;
- virDomainEventGraphicsSubjectPtr subject;
- } graphics;
- struct {
- char *path;
- int type;
- int status;
- } blockJob;
- struct {
- char *oldSrcPath;
- char *newSrcPath;
- char *devAlias;
- int reason;
- } diskChange;
- struct {
- char *devAlias;
- int reason;
- } trayChange;
- struct {
- /* In unit of 1024 bytes */
- unsigned long long actual;
- } balloonChange;
- struct {
- char *devAlias;
- } deviceRemoved;
- } data;
-};
One of the things that I think was very sub-optimal about our
design originally was this approach of using huge unions for
the event specific data.
This only gets worse because we now also need unions for the
virDomainMeta structure too.
Since we originally designed this events code, we have gained
the ability to have a simple object hierchies.
As such I think I'd be inclined to make use of this to make
the code a bit more modular, which may also help us avoid
some of this bulk renaming you had todo.
I'd say we go for a base class of virObjectEvent which just
contains a unique integer event ID value.
We then have a virDomainEvent which inherits from this
and adds the virDomainMeta identity information
Finally we can have subclasses for each event type eg
virDomainEventGraphics, virDomainEventLifecycle and
so on, which contain the event specific data.
The way you've split the methods between domain_event.h and
object_event.h makes sense here.
To make patch easier to review though, I'd suggest that you
have one patch which only renames the methods:
s/virDomainEventState/virObjectEventState/
then another patch which replaces the union approach with
a virObject subclass based approach, again all in domain_conf.h.
Then finally move the bits named virObjectEventXXXX into
the new object_event.h file.
The key idea being to just keep code movement separate
from code renaming.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|