8:23 a.m.
Enclosed is a cleanup patch I applied, it corrects the XML api generation,
fixes warning in the XSLT stylesheet, add comments to a number of internal
functions, makes most function of the QEmu back-end static, fixes a few bugs
found in the way.
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/
8:40 a.m.
Daniel Veillard wrote:
Enclosed is a cleanup patch I applied, it corrects the XML api
generation,
fixes warning in the XSLT stylesheet, add comments to a number of internal
functions, makes most function of the QEmu back-end static, fixes a few bugs
found in the way.
Looks fine, except my general grumble below.
@@ -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.
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 ...
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
9:27 a.m.
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/
6388
days inactive
6388
days old
2 comments
2 participants
participants (2)
-
Daniel Veillard -
Richard W.M. Jones