Re: [libvirt] [PATCH 1/5] Add public API to register a callback to be invoked on connection close
On Thu, Jul 19, 2012 at 11:46:28AM -0600, Eric Blake wrote:
On 07/19/2012 09:04 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange(a)redhat.com>
>
> Define a new virConnectSetCloseCallback() public API which allows
> registering a callback to be invoked when the connection to a
> hypervisor is closed. The callback is provided with the reason for
> the close, which may be 'error', 'eof' or 'keepalive'.
>
> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
> ---
> include/libvirt/libvirt.h.in | 40 +++++++++++++++++++++++--------
> src/datatypes.c | 4 ++++
> src/datatypes.h | 5 ++++
> src/libvirt.c | 53 ++++++++++++++++++++++++++++++++++++++++++
> src/libvirt_public.syms | 6 +++++
> 5 files changed, 98 insertions(+), 10 deletions(-)
>
> +typedef enum {
> + VIR_CONNECT_CLOSE_REASON_ERROR = 1, /* Misc I/O error */
Any reason you skipped 0? It will make wrapping this in a VIR_ENUM harder.
No, that's a mistake.
> +int virConnectSetCloseCallback(virConnectPtr conn,
> + virConnectCloseFunc cb,
> + void *opaque,
> + virFreeCallback freecb);
Do we want a 'flags' argument?
No, I don't think so for the callbacks
> +++ b/src/datatypes.c
> @@ -115,6 +115,10 @@ virReleaseConnect(virConnectPtr conn) {
>
> virMutexLock(&conn->lock);
>
> + if (conn->closeOpaque &&
NACK to this half of the condition. A client should be allowed to pass
NULL as their opaque data.
This doesn't prevent them passing NULL. The 'virFreeFunc' callback is
for free'ing the 'opaque' data. If the opaque data is NULL, there is
nothing that requires free'ing.
> +int virConnectSetCloseCallback(virConnectPtr conn,
> + virConnectCloseFunc cb,
> + void *opaque,
> + virFreeCallback freecb)
> +{
> + VIR_DEBUG("conn=%p", conn);
> +
> + virResetLastError();
> +
> + if (!VIR_IS_CONNECT(conn)) {
> + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> + virDispatchError(NULL);
> + return -1;
> + }
> +
> + virMutexLock(&conn->lock);
> +
> + if (conn->closeOpaque &&
> + conn->closeFreeCallback)
> + conn->closeFreeCallback(conn->closeOpaque);
Again, no need to check closeOpaque, NULL is valid there.
Same point as above.
> +
> + conn->closeCallback = cb;
> + conn->closeOpaque = opaque;
> + conn->closeFreeCallback = freecb;
So the user can call this as many times as they want to override or
uninstall an existing registered callback? I guess that works.
Alternatively I could raise an error, probably nicer than silently
overriding an existing callback
ACK, once you allow a NULL opaque pointer, and answer why a flags is not
useful (or else add a flags).
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 :|