[libvirt] [PATCH 2/2] Java bindings for domain events
This patch allows the remote driver to work with an asynchronous
EventImpl (it's the only one using an externally-supplied one), assuming
libvirt is compiled with pthread support. (Without pthreads, this code
is harmless in a single-threaded environment.)
Basically it uses a mutex to protect reads from the RPC socket in such a
way that message reads (in their entirety) are done atomically
(otherwise the remoteDomainEventFired() can race the call() code that
reads replies events).
In addition, I update the EventImpl handle to prevent
remoteDomainEventFired() from being called everytime a reply is sent.
(This helps us dispatch events in a timely manner, though it's not
strictly necessary. Without it, any events coming in during a call()
won't be dispatched until the call drops the socket lock (because
remoteDomainEventFired() will be stuck awaiting the lock).
Dave
diff --git a/src/remote_internal.c b/src/remote_internal.c
index 2ca7930..59128f6 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -116,6 +116,7 @@ struct private_data {
virDomainEventQueuePtr domainEvents;
/* Timer for flushing domainEvents queue */
int eventFlushTimer;
+PTHREAD_MUTEX_T(lock); /* Serializes socket reads w/async EventImpl */
};
#define GET_PRIVATE(conn,retcode) \
@@ -700,6 +701,9 @@ doRemoteOpen (virConnectPtr conn,
} /* switch (transport) */
+/* This must precede the first call() */
+priv-eventFlushTimer = -1;
+
/* Try and authenticate with server */
if (remoteAuthenticate(conn, priv, 1, auth, authtype) == -1)
goto failed;
@@ -744,6 +748,8 @@ doRemoteOpen (virConnectPtr conn,
}
}
+pthread_mutex_init(priv-lock, NULL);
+
if(VIR_ALLOC(priv-callbackList)0) {
error(conn, VIR_ERR_INVALID_ARG, _(Error allocating callbacks list));
goto failed;
@@ -1250,6 +1256,8 @@ doRemoteClose (virConnectPtr conn, struct private_data *priv)
/* Free queued events */
virDomainEventQueueFree(priv-domainEvents);
+pthread_mutex_destroy(priv-lock);
+
return 0;
}
@@ -4536,11 +4544,11 @@ static int really_read (virConnectPtr conn, struct private_data *priv,
* else Bad Things will happen in the XDR code.
*/
static int
-call (virConnectPtr conn, struct private_data *priv,
- int flags /* if we are in virConnectOpen */,
- int proc_nr,
- xdrproc_t args_filter, char *args,
- xdrproc_t ret_filter, char *ret)
+really_call (virConnectPtr conn, struct private_data *priv,
+ int flags /* if we are in virConnectOpen */,
+ int proc_nr,
+ xdrproc_t args_filter, char *args,
+ xdrproc_t ret_filter, char *ret)
{
char buffer[REMOTE_MESSAGE_MAX];
char buffer2[4];
@@ -4596,16 +4604,18 @@ call (virConnectPtr conn, struct private_data *priv,
really_write (conn, priv, flags REMOTE_CALL_IN_OPEN, buffer, len-4) == -1)
return -1;
+pthread_mutex_lock(priv-lock);
+
retry_read:
/* Read and deserialise length word. */
if (really_read (conn, priv, flags REMOTE_CALL_IN_OPEN, buffer2, sizeof buffer2) == -1)
-return -1;
+goto unlock_return_err;
xdrmem_create (xdr, buffer2, sizeof buffer2, XDR_DECODE);
if (!xdr_int (xdr, len)) {
error (flags REMOTE_CALL_IN_OPEN ? NULL : conn,
VIR_ERR_RPC, _(xdr_int (length word, reply)));
-return -1;
+goto unlock_return_err;
}
xdr_destroy (xdr);
@@ -4615,12 +4625,14 @@ retry_read:
if (len 0 || len REMOTE_MESSAGE_MAX) {
error (flags REMOTE_CALL_IN_OPEN ? NULL : conn,
VIR_ERR_RPC, _(packet received from server too large));
-return -1;
+goto unlock_return_err;
}
/* Read reply header and what follows (either a ret or an error). */
if (really_read (conn, priv, flags REMOTE_CALL_IN_OPEN, buffer, len) == -1)
-return -1;
+goto unlock_return_err;
+
+pthread_mutex_unlock(priv-lock);
/* Deserialise reply header. */
xdrmem_create (xdr, buffer, len, XDR_DECODE);
@@ -4729,8 +4741,33 @@ retry_read:
xdr_destroy (xdr);
return -1;
}
+
+ unlock_return_err:
+pthread_mutex_unlock(priv-lock);
+return -1;
+}
+
+static int call (virConnectPtr conn, struct private_data *priv,
+ int flags /* if we are in virConnectOpen */,
+ int proc_nr,
+ xdrproc_t args_filter, char *args,
+ xdrproc_t ret_filter, char *ret)
+{
+int rv;
+if (priv-eventFlushTimer = 0)
+virEventUpdateHandle(priv-sock, 0);
+rv = really_call(conn, priv, flags, proc_nr,
+ args_filter, args,
+ ret_filter, ret);
+if (priv-eventFlushTimer = 0)
+virEventUpdateHandle(priv-sock,
+ VIR_EVENT_HANDLE_READABLE |
+
Re: [libvirt] [PATCH 2/2] Java bindings for domain events
On Tue, Nov 18, 2008 at 11:18:38AM -0500, David Lively wrote: This patch allows the remote driver to work with an asynchronous EventImpl (it's the only one using an externally-supplied one), assuming libvirt is compiled with pthread support. (Without pthreads, this code is harmless in a single-threaded environment.) I don't like this patch, since it is only making one tiny part of the API thread-safe, for one tiny use case. eg, if someone uses events from the Xen driver in Java with threads, they'll crash burn, since only the remote driver is being protected. IMHO, if we want to allow multi-threaded access to a single virConnectPtr object then we need to do the full job and make the whole thing safe, including adding thread-local error objects, to replace the inherantly non-thread safe global per-virConnect error objects. The other alternative that we've discussed in the past is to add the ability to 'clone' an existing virConnectPtr object. The idea being that a 2nd thread could be given a clone, and safely use that. Internally we'd share synchronize on relevant state information, so we didn't actually need to setup a separate network connection re-auth for each. As it stands, this patch adding semi-thread safe access will make it harder for us to pursue this 2nd option of cloning virConnect, without breaking the Java bindings in future. Basically it uses a mutex to protect reads from the RPC socket in such a way that message reads (in their entirety) are done atomically (otherwise the remoteDomainEventFired() can race the call() code that reads replies events). Why doesn't the Java code simply synchronize on the virConnect object before invoking the FD event callbacks. That will ensure another thread has finished whatever API call it was doing In addition, I update the EventImpl handle to prevent remoteDomainEventFired() from being called everytime a reply is sent. (This helps us dispatch events in a timely manner, though it's not strictly necessary. Without it, any events coming in during a call() won't be dispatched until the call drops the socket lock (because remoteDomainEventFired() will be stuck awaiting the lock). I don't really like the idea of applying any of this patch, since I think it'll cause us unexpected complications when doing proper thread support in the next release of libvirt, and risk us breaking the java bindings. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Java bindings for domain events
On Tue, 2008-11-18 at 17:05 +, Daniel P. Berrange wrote: On Tue, Nov 18, 2008 at 11:18:38AM -0500, David Lively wrote: This patch allows the remote driver to work with an asynchronous EventImpl (it's the only one using an externally-supplied one), assuming libvirt is compiled with pthread support. (Without pthreads, this code is harmless in a single-threaded environment.) I don't like this patch, since it is only making one tiny part of the API thread-safe, for one tiny use case. eg, if someone uses events from the Xen driver in Java with threads, they'll crash burn, since only the remote driver is being protected. Currently ONLY the remote driver (and yes, soon - the xen driver, which would also need thread-safety changes) use an EventImpl supplied externally. All others use the libvirtd-provided synchronous impl. Why doesn't the Java code simply synchronize on the virConnect object before invoking the FD event callbacks. That will ensure another thread has finished whatever API call it was doing Note that the Java code uses (Java's builtin) Connect-level lock to avoid concurrent calls using the same virConnect. This even applies to domain event delivery - note that Connect.fireDomainEventCallbacks is (in the Java patch) a synchronized method. A FD event callback isn't associated with a particular virConnect object, and EventImpls aren't virConnect-specific, so I can't lock the virConnect (see below). When we exposed virEventRegisterImpl, we said that externally-supplied event impls must not make callbacks when a virConnect is being used. If the Java EventImpl were following this rule, we wouldn't need this patch. BUT because the EventImpl can't know which virConnect (if any) is involved in a particular callback, the only way to satisfy this rule is to lock ALL Connect objects before making an EventImpl callback. This is (IMO) both way too restrictive, and (I'm a little less sure of this) not restrictive enough. (I suspect we'd find that making two callbacks concurrently can break things, even if no virConnects are in use at the time.) I think we have to allow externally supplied EventImpls to make callbacks without regard to which virConnect, etc. objects are in use, since EventImpls don't know anything about virConnect, etc. This doesn't require all of libvirt to be made thread-safe. So I view this fix (and the one necessary for the xen driver once it starts using an externally-supplied EventImpl) as providing just enough concurrency control to allow an asynchronous EventImpl, while still making the libvirt user (the Java bindings, in my case) responsible for avoiding concurrent virConnect usage. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
