<div dir="ltr"><div class="gmail_extra">Hi Claudio<br><br></div><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 3, 2014 at 3:00 PM, Claudio Bley <span dir="ltr"><<a href="mailto:cbley@av-test.de" target="_blank">cbley@av-test.de</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">At Thu, 27 Feb 2014 03:47:31 +0000,<br>
<div class="">Chris Ellis wrote:<br>
><br>
> Hi all<br>
><br>
> I'm new to this list, I've been making use of the Libvirt Java bindings<br>
> recently. I wanted to make use of domain events yesterday<br>
> so my application can be alerted when the state of a domain changes etc.<br>
><br>
> However I quickly discovered that domain events are completely broken in<br>
> the current Java bindings.<br>
<br>
</div><div class="">> So I have implemented support for domain events in the Java<br>
> bindings.<br>
<br>
</div>Searching the mailing list or even asking on the list before<br>
implementing something would have probably saved you some work...<br></blockquote><div><br></div><div>Arguably I should have spend longer looking, I did google around but <br>obviously not well enough. From my perspective, my evenings worth <br>
of effort isn't wasted, I've learnt more of the libvirt internals and advanced <br>my own project, I can easily backtrack to make use of your code.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class=""><br>
> Currently I've only implemented the following domain<br>
> event IDs:<br>
><br>
> * VIR_DOMAIN_EVENT_ID_LIFECYCLE<br>
> * VIR_DOMAIN_EVENT_ID_REBOOT<br>
> * VIR_DOMAIN_EVENT_ID_RTC_CHANGE<br>
<br>
</div>As Daniel already pointed out, there's my patch set here<br>
<a href="https://www.redhat.com/archives/libvir-list/2014-February/msg00823.html" target="_blank">https://www.redhat.com/archives/libvir-list/2014-February/msg00823.html</a><br>
which contains event support for I/O error, life cycle, PM wakeup, PM<br>
suspend and reboot events (because that's what I needed).<br>
<div class=""><br>
> Implementing these is enough to test that my implementation is sane,<br>
> I'm hoping to implement the majority of the other events<br>
> soon.<br>
><br>
> Events can be listened to by, registering via the Connect object, as<br>
> follows:<br>
><br>
> con.domainEventRegisterAny(DomainEventID.VIR_DOMAIN_EVENT_ID_LIFECYCLE,<br>
> new DomainLifecycleEventHandler() {<br>
> @Override<br>
> public void onStarted(Connect connection, Domain domain, DomainEventType<br>
> event, DomainEventStartedDetailType detail) throws LibvirtException {<br>
> System.out.println("Got start event: " + event + "::" + detail + " for<br>
> domain " + domain.getName());<br>
> }<br>
> });<br>
<br>
</div>As Eric pointed out, there's no need to strictly adhere to the<br>
function naming of libirt's C API. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I dislike overloading though (because it makes the code ambiguous and<br>
doesn't buy you much).<br></blockquote><div><br></div><div>I had been trying to keep my code inline with the C API, as I find libvirt-java a little <br>confused over whether it is Java like or C like.<br></div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class=""><br>
> I've put my clone of the libvirt-java git repository on Github, my<br>
> modifications to the Java binding are in a separate branch and<br>
> should be simple to merge. The changes can be viewed at:<br>
><br>
> <a href="https://github.com/intrbiz/libvirt-java/compare/master...ce-domain-events" target="_blank">https://github.com/intrbiz/libvirt-java/compare/master...ce-domain-events</a><br>
<br>
</div>I've had a look at it and I see a few similarities to my<br>
implementation. So, we're on the right track, I guess... :-)<br>
<br>
For my implementation I've chosen to put the focus on simplicity for<br>
the user and I tried not to limit the usage of the public interface in<br>
any way. In particular this means:<br>
<br>
- using the usual Java term "listener" and add/remove(Listener) idiom<br>
<br>
- do not give "null" a special meaning when passed as a domain<br>
parameter of a method of the public API<br>
(instead, connect.add*Listener(..) registers a listener for all<br>
domains of a connection, whereas domain.add*Listener(..) only for<br>
the domain at hand)<br>
<br>
- the user is free to implement several listeners in one class,<br>
reacting on different events in a central place<br>
(this is not possible with your approach and would not be possible<br>
when using method overloading)<br>
<br>
- keep the /callback/ method's argument list as short as possible<br>
(e.g. in your DomainLifecycleEventHandler there's an onStarted method<br>
which takes 5 arguments, but it would actually only require two --<br>
the domain and the event detail)<br>
<br>
- hide the underlying implementation as far as possible<br>
(the user shouldn't be bothered with callback IDs, event IDs et cetera)<br>
<br>
- do not run an event loop in a different thread by default<br>
(this might not be what the user wants, for several reasons)<br></blockquote><div><br></div><div>This is 6 of one, half a dozen of the other. IMHO the majority of users <br></div><div>probably just want the library to work, rather than having to faff with how <br>
to execute the event loop.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- do not specify a throws LibvirtException clause for any of the<br>
listener methods<br>
(this is actually forcing the user to handle any such exception<br>
right on the spot in the listener method itself (or wrap the<br>
LibvirtException into some kind of unchecked exception which gets<br>
handled by the JNA callback exception handler))<br>
<br>
Another difference to your implementation is the handling of the<br>
different event types for the live cycle events. You have chosen to<br>
branch on the events and implement a method for each type of event. I<br>
like the idea in general (because it solves the problem of<br>
transporting the event detail in regard to the event type to the<br>
user's code in a type safe way), but to me it seems a bit too<br>
complicated if the user wants to handle some type of events in the<br>
same way. A simple switch statement would be a lot easier instead of<br>
branching on the event types and then merging the code paths together<br>
again. Of course, the user could override the onEvent method in that<br>
case but one has to plan which approach beforehand.<br>
<br>
I'm in favor of giving the user only one approach, not both. If it<br>
seems feasible the user could implement the switch on the event type<br>
herself.<br>
<br>
But maybe a LifecycleAdapter class could be added for convenience<br>
which does the branching...<br></blockquote><div><br></div><div>It would make sense to go with the adapter approach, I like the separating <br></div><div>out of events rather than forcing the user to switch. I tend to favour offering <br>
</div><div>flexibility rather than the one size fits all.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[See, nobody knows what I'm talking about without seeing the<br>
code. That's why we like to have the patches directly send to the<br>
list.]<br>
<div class=""><br>
> I'm keen to get these enhancements / fixes merged into libvirt-java.<br>
<br>
</div>My patches were excepted already, but I'll wait for a short term if<br>
you want to comment on them...!?<br>
<br>
The plan would be to get version 1.5.2 out after the dust has settled<br>
and maybe a few people have tested the API.<br></blockquote><div><br></div><div>I think it is best for you to merge your changes into master, as you've got <br>a fair raft of changes there.<br></div><div><br></div><div>
I'm happy to provide some testing. I've been seeing some issues, were by <br>the JVM is segfaulting within virDomainFree(). I'd like to review how your <br>code handles the freeing of the Domain objects passed to an event handler.<br>
</div><br></div><div class="gmail_quote">Hopefully this might be solved by your Domain.constructIncRef() functionality.<br></div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class=""><br>
> I would also like to submit further fixes to race conditions<br>
> in the free() handling.<br>
<br>
</div>Patches are very welcome! See the instructions here:<br>
<a href="http://libvirt.org/hacking.html#patches" target="_blank">http://libvirt.org/hacking.html#patches</a> which apply to libvirt-java<br>
also, but use "[java]" or "[libvirt-java]" in the subject to indicate<br>
the focus.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><font color="#888888"><br>
Claudio<br>
</font></span></blockquote></div><br></div><div class="gmail_extra">Chris<br></div></div>