"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list