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