Re: [Libvir] Cleanup patch
On Fri, Jun 29, 2007 at 02:40:23PM +0100, Richard W.M. Jones wrote:
>@@ -113,12 +128,19 @@ virBufferFree(virBufferPtr buf)
> * virBufferContentAndFree:
> * @buf: Buffer
> *
>- * Return the content from the buffer and free (only) the buffer
>structure.
>+ * Get the content from the buffer and free (only) the buffer structure.
>+ *
>+ * Returns the buffer content or NULL in case of error.
> */
> char *
> virBufferContentAndFree (virBufferPtr buf)
> {
>- char *content = buf->content;
>+ char *content;
>+
>+ if (buf == NULL)
>+ return(NULL);
>+
>+ content = buf->content;
>
> free (buf);
> return content;
I know we do this sort of thing all over the place, but it's bad
practice (IMHO). If someone passes a NULL into this function then it's
an error, and it's better to segfault early rather than compound or hide
the error.
The error should be raised before. If it wasn't then there is a programming
bug. And a library must not segfault on hazard like out of memory, sorry no !
Like we tests return from malloc() I think we must shield the internal code
as much as possible. In any case segfaulting is not the proper way to
raise a problem.
Of course I wish we were using a language where you could specify
these
rules statically, and in fact I've been analysing the libvirt code using
some static analysis tools to try & find these types of bugs
automatically. Results later ...
Fixing bugs, yes definitely, that I can only agree with.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/