"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Fri, Jan 16, 2009 at 12:11:16PM +0000, Daniel P. Berrange wrote:
@@ -114,6 +164,11 @@ struct private_data { virDomainEventQueuePtr domainEvents; /* Timer for flushing domainEvents queue */ int eventFlushTimer; + + /* List of threads currently doing dispatch */ + int wakeupSend; + int wakeupRead;
How about appending "FD" to indicate these are file descriptors. The names combined with the comment (which must apply to waitDispatch) made me wonder what they represented. Only when I saw them used in safewrite /saferead calls did I get it.
Yes, good idea - and its not really a list of threads either, so the comment is a little misleading :-)
+ /* Encode the length word. */ + xdrmem_create (&xdr, rv->buffer, 4, XDR_ENCODE); + if (!xdr_int (&xdr, (int *)&rv->bufferLength)) { + error (flags & REMOTE_CALL_IN_OPEN ? NULL : conn, VIR_ERR_RPC, + _("xdr_int (length word)"));
I haven't done enough xdr* work to know, and man pages didn't provide an immediate answer: Is there no need to call xdr_destroy on this error path? I'd expect xdrmem_create to do any allocation, not xdr_int. There are many like this.
Yes, the 'error:' codepath should be calling 'xdr_destroy(&xdr)' to ensure free'ing of memory.
+ goto error; + } + xdr_destroy (&xdr); + + return rv; + +error: + VIR_FREE(ret); + return NULL;
The above should free rv, not ret:
VIR_FREE(rv);
Here is an update with those suggested renames & bug fixes in it.
It also addresses the error reporting issue mentioned in
http://www.redhat.com/archives/libvir-list/2009-January/msg00428.html
That code should not have been using DEBUG() - it now correctly raises a real error including the error string, not just errno. There were two other bugs with missing error raising in the path for sasl_encode/decode.
Everything upto this patch is committed, so this is diffed against current CVS.
All looks fine, but for a reverted change and some added strerror uses. While merging with your earlier changes (I effectively reverted the old 8/25 on a new branch, replacing it with this one and then rebased the remaining change sets), I got this conflict <<<<<<< HEAD:src/remote_internal.c if (pipe(wakeupFD) < 0) { errorf (conn, VIR_ERR_SYSTEM_ERROR, _("unable to make pipe %s"), strerror(errno)); ======= if (pipe(wakeup) < 0) { virReportSystemError(conn, errno, "%s", _("unable to make pipe")); >>>>>>> 03e5096... Remove use of strerror():src/remote_internal.c that suggests that your new wakeupFD-using change reintroduces a use of strerror that was previously removed. But it's no big deal, since we're planning to clean up the remaining strerror uses pretty soon.