[Libvir] PATCH 2/4: Xen driver support for serial/paralle

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> wrote:

> On Fri, Apr 25, 2008 at 03:11:44AM -0400, Daniel Veillard wrote:
>> On Thu, Apr 24, 2008 at 10:14:49PM +0100, Daniel P. Berrange wrote:
>> > On Thu, Apr 24, 2008 at 10:42:34PM +0200, Jim Meyering wrote:
>> > > All of the virBufferAddLit and virBufferVSprintf calls
>> > > above can fail with ENOMEM.  I see that there are *many* more
>> > > virBufferAddLit and virBufferVSprintf calls that ignore their
>> > > return values (over 300), so maybe I'm missing something.
>> >
>> > Yes, the entire set of Xen drivers is basically fubar wrt to checking
>> > virBufferXXX return values. I felt that needed addresing independantly
>> > changing everything in one go, so didn't try to address that in this
>> > patch.
>>
>>   Maybe the best is to store in the buffer the fact that there have been
>> an error at some point, and check that value before extracting the content
>> in a single place, that would make the code cleaner, and makes harder to
>> miss one test.
>
> So we've two options:
>
>  - Make all the virBuffer* functions be void, and keep an internal
>    error flag to be checked at the end
>
>  - Annotate all the virBuffer* functions with __attribute((warn_unused_result))
>    which forces the caller to check the return value
>
> I'm acutally inclined to go for the former, since it'll let us remove a
> large number of 'goto no_memory' statements giving clearer code

I agree.

> I think I'd also like to mak the virBuffer struct private, so people
> can't access the data field directly - force them to go via an API to
> get hold of the internal char * - this lets us ensure they don't access
> the data if an error has occurred.

Sounds good.  As a second step, I guess.
If we're revamping, then it'd be good to move away from the
pointer-hiding typedef in the current implementation:

    typedef virBuffer *virBufferPtr;

That makes it hard to declare such a parameter const,
as it will have to be in the string-extracting or
error-state-querying functions.

The alternative is to use e.g.,

    typedef const virBuffer *virBufferConstPtr;

but that's far less readable and doesn't scale well.