events: Return values should use golang idioms

[purpleidea]

A golang lib should use idiomatic return values of error and nil instead of int as is common in C. This updates these functions to return as expected.

[purpleidea]

Dear reviewers, this is my first real patch to this lib, and while I am pretty familiar with golang, my C knowledge is weaker. Please make sure I don't make any "C" errors.

Thanks!

[rgbkrk]

/cc @vincentbernat this looks like an area you know well

[rgbkrk]

Thanks for the patch @purpleidea!

[purpleidea]

@rgbkrk NP. It's a fairly small patch, but my comment was meant also as an introductory hello.

[vincentbernat]

Usually, returning GetLastError() will give a proper error message and will avoid to have to build error messages (with embedded numbers).

Moreover, this breaks the current API. This is unfortunate but correcting that would need to have a "v3" API. I think that currently such changes are too small to warrant to maintain a v3 branch. But we could collect all of them (maybe by tagging them with labels in GitHub). Once we have a few of them, we can start maintaining this second branch. After some time, the v2 could be deprecated. I understand this is how things are done in Go but I have little experience on the matter.

[purpleidea]

@vincentbernat Since it's nearly impossible to maintain such little patches (and there are many to come) until we're ready to switch the a V3 API, here is what is usually done and recommended:

git master (where this patch applies) is the dev branch where everything goes. If you wish to have a V2 not breaking API branch, fork that off of git master and keep working on that.

Otherwise it's impossible to get these changes in.

[vincentbernat]

The project is advertised to work through gopkg.in, so I suppose users expect to have a versioned API. We could move to v3 now, but we need to fix most of the API problems in the process to avoid to get v4 in a month. Moreover, maybe it would be nice to continue to fix bugs in a v2 branch, but it seems there none in the last few months, so we could just release the ultimate v2 version and move on to v3.

[purpleidea]

@vincentbernat gopkg.in works exactly for the case I described. Point it at the v2 branch, and keep working on that. As for git master, that's where patches like this one should land. When we want to create a V3 we just branch or tag a v3 from a git master commit.

[vincentbernat]

OK, fine by me then. The PR still needs to be updated to use GetLastError().

[purpleidea]

@vincentbernat Okay, I've updated the patch to use those...

Doesn't this sort of GetLastError thing guarantee that this lib can never be thread safe? (I'm possibly missing some context, in particular around the libvirt internals.)

[vincentbernat]

libvirt uses thread local storage to store the last error message, so it's safe from this point of view. cgo will lock the goroutine on the current thread when executing C code. I don't know if:

• Go can move the goroutine to another thread after the C call and before the call to virGetLastError()
• Go can spawn another goroutine from the current one in the same thread, after the C call and before the call to virGetLastError(), this goroutine would be able to do another libvirt call and therefore alter the result of virGetLastError() for the current thread.

The first problem could be worked around by locking the goroutine to the thread using runtime.LockOsThread(). I think there is no solution for the second problem (which is a similar problem to the inability to use setuid() safely in Go). But maybe the scheduler has some guarantee in the absence of blocking calls.

[purpleidea]

@vincentbernat Okay, thanks for the info. In any case, it's probably something that's worth looking into in another PR.

I think if nobody has any complaints, this is ready to merge.

[vincentbernat]

OK. I have created the v2 branch. Maybe we should also release a v2.13.0 with the tip of this branch. It has been more than a year since the last time a new version was released. I don't know if there is any checker that we should run before tagging.

[purpleidea]

w00t! thanks! I think it's good to now have this WIP, API breakable git master so that we can get lots of useful fixes in and make progress. Here's my starting point. Comments welcome: https://github.com/rgbkrk/libvirt-go/issues/116

Thanks!

[rgbkrk]

Great, thanks for the collaboration @vincentbernat and @purpleidea.

[vincentbernat]

So, I have released a 2.13.0.