[libvirt] Libvirt Java Bindings - adding domain event support - pull request

Mon Mar 3 21:29:56 UTC 2014

Hi Claudio

On Mon, Mar 3, 2014 at 3:00 PM, Claudio Bley <cbley at av-test.de> wrote:

> At Thu, 27 Feb 2014 03:47:31 +0000,
> Chris Ellis wrote:
> >
> > Hi all
> >
> > I'm new to this list, I've been making use of the Libvirt Java bindings
> > recently.  I wanted to make use of domain events yesterday
> > so my application can be alerted when the state of a domain changes etc.
> >
> > However I quickly discovered that domain events are completely broken in
> > the current Java bindings.
>
> > So I have implemented support for domain events in the Java
> > bindings.
>
> Searching the mailing list or even asking on the list before
> implementing something would have probably saved you some work...
>

Arguably I should have spend longer looking, I did google around but
obviously not well enough.  From my perspective, my evenings worth
of effort isn't wasted, I've learnt more of the libvirt internals and
advanced
my own project, I can easily backtrack to make use of your code.

>
> > Currently I've only implemented the following domain
> > event IDs:
> >
> >  * VIR_DOMAIN_EVENT_ID_LIFECYCLE
> >  * VIR_DOMAIN_EVENT_ID_REBOOT
> >  * VIR_DOMAIN_EVENT_ID_RTC_CHANGE
>
> As Daniel already pointed out, there's my patch set here
> https://www.redhat.com/archives/libvir-list/2014-February/msg00823.html
> which contains event support for I/O error, life cycle, PM wakeup, PM
> suspend and reboot events (because that's what I needed).
>
> > Implementing these is enough to test that my implementation is sane,
> > I'm hoping to implement the majority of the other events
> > soon.
> >
> > Events can be listened to by, registering via the Connect object, as
> > follows:
> >
> >  con.domainEventRegisterAny(DomainEventID.VIR_DOMAIN_EVENT_ID_LIFECYCLE,
> > new DomainLifecycleEventHandler() {
> >    @Override
> >    public void onStarted(Connect connection, Domain domain,
> DomainEventType
> > event, DomainEventStartedDetailType detail) throws LibvirtException {
> >      System.out.println("Got start event: " + event + "::" + detail + "
> for
> > domain " + domain.getName());
> >    }
> >  });
>
> As Eric pointed out, there's no need to strictly adhere to the
> function naming of libirt's C API.

> I dislike overloading though (because it makes the code ambiguous and
> doesn't buy you much).
>

I had been trying to keep my code inline with the C API, as I find
libvirt-java a little
confused over whether it is Java like or C like.

>
> > I've put my clone of the libvirt-java git repository on Github, my
> > modifications to the Java binding are in a separate branch and
> > should be simple to merge.  The changes can be viewed at:
> >
> >
> https://github.com/intrbiz/libvirt-java/compare/master...ce-domain-events
>
> I've had a look at it and I see a few similarities to my
> implementation. So, we're on the right track, I guess... :-)
>
> For my implementation I've chosen to put the focus on simplicity for
> the user and I tried not to limit the usage of the public interface in
> any way. In particular this means:
>
> - using the usual Java term "listener" and add/remove(Listener) idiom
>
> - do not give "null" a special meaning when passed as a domain
>   parameter of a method of the public API
>   (instead, connect.add*Listener(..) registers a listener for all
>    domains of a connection, whereas domain.add*Listener(..) only for
>    the domain at hand)
>
> - the user is free to implement several listeners in one class,
>   reacting on different events in a central place
>   (this is not possible with your approach and would not be possible
>    when using method overloading)
>
> - keep the /callback/ method's argument list as short as possible
>   (e.g. in your DomainLifecycleEventHandler there's an onStarted method
>    which takes 5 arguments, but it would actually only require two --
>    the domain and the event detail)
>
> - hide the underlying implementation as far as possible
>   (the user shouldn't be bothered with callback IDs, event IDs et cetera)
>
> - do not run an event loop in a different thread by default
>   (this might not be what the user wants, for several reasons)
>

This is 6 of one, half a dozen of the other.  IMHO the majority of users
probably just want the library to work, rather than having to faff with how
to execute the event loop.

>
> - do not specify a throws LibvirtException clause for any of the
>   listener methods
>   (this is actually forcing the user to handle any such exception
>    right on the spot in the listener method itself (or wrap the
>    LibvirtException into some kind of unchecked exception which gets
>    handled by the JNA callback exception handler))
>
> Another difference to your implementation is the handling of the
> different event types for the live cycle events. You have chosen to
> branch on the events and implement a method for each type of event.  I
> like the idea in general (because it solves the problem of
> transporting the event detail in regard to the event type to the
> user's code in a type safe way), but to me it seems a bit too
> complicated if the user wants to handle some type of events in the
> same way. A simple switch statement would be a lot easier instead of
> branching on the event types and then merging the code paths together
> again. Of course, the user could override the onEvent method in that
> case but one has to plan which approach beforehand.
>
> I'm in favor of giving the user only one approach, not both. If it
> seems feasible the user could implement the switch on the event type
> herself.
>
> But maybe a LifecycleAdapter class could be added for convenience
> which does the branching...
>

It would make sense to go with the adapter approach, I like the separating
out of events rather than forcing the user to switch.  I tend to favour
offering
flexibility rather than the one size fits all.

>
> [See, nobody knows what I'm talking about without seeing the
>  code. That's why we like to have the patches directly send to the
>  list.]
>
> > I'm keen to get these enhancements / fixes merged into libvirt-java.
>
> My patches were excepted already, but I'll wait for a short term if
> you want to comment on them...!?
>
> The plan would be to get version 1.5.2 out after the dust has settled
> and maybe a few people have tested the API.
>

I think it is best for you to merge your changes into master, as you've got
a fair raft of changes there.

I'm happy to provide some testing.  I've been seeing some issues, were by
the JVM is segfaulting within virDomainFree().  I'd like to review how your
code handles the freeing of the Domain objects passed to an event handler.

Hopefully this might be solved by your Domain.constructIncRef()
functionality.

>
> > I would also like to submit further fixes to race conditions
> > in the free() handling.
>
> Patches are very welcome! See the instructions here:
> http://libvirt.org/hacking.html#patches which apply to libvirt-java
> also, but use "[java]" or "[libvirt-java]" in the subject to indicate
> the focus.
>

> Claudio
>

Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140303/1fb52f6a/attachment-0001.htm>