[libvirt] PATCH: 8/25: Concurrent dispatch of RPC methods

[Jim Meyering]

"Daniel P. Berrange" <berrange at 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.