Re: [libvirt] PATCH: 8/25: Concurrent dispatch of RPC methods
"Daniel P. Berrange" <berrange(a)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.