Re: [Libvir] Cleanup patch

[Daniel Veillard <veillard redhat com>]

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 redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/