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