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