[Libvir] PATCH: Make the virBuffer API harder to misuse

[Daniel Veillard]

On Sat, Apr 26, 2008 at 05:37:11PM +0100, Daniel P. Berrange wrote:
> 
> 
> The following set of changes adjust the way errors are handled in the
> virBuffer routines. The key idea is to make it hard (impossible) to
> misuse the API, with each change addressing one of the errors scenarios
> I've found in existing code using the routines.

  In general I agree an like the change, except

>  - The contents of the struct are no longer public.
> 
>    Rationale: This stops people accessing the buffer directly,
>    thus preventing use of data which may be in an error state.

 results in this:

> Index: src/buf.h
> ===================================================================
> RCS file: /data/cvs/libvirt/src/buf.h,v
> retrieving revision 1.5
> diff -u -p -r1.5 buf.h
> --- src/buf.h	20 Feb 2008 15:29:13 -0000	1.5
> +++ src/buf.h	26 Apr 2008 16:24:38 -0000
> @@ -20,22 +20,45 @@
>   */
>  typedef struct _virBuffer virBuffer;
>  typedef virBuffer *virBufferPtr;
> +
> +#ifndef __VIR_BUFFER_C__
> +/* This constant must match size of the actual struct
> +   in the buf.c file */
> +#if __WORDSIZE == 64
> +#define __SIZEOF_VIR_BUFFER 32
> +#define VIR_BUFFER_INITIALIZER { { 0, 0, 0, 0,  \
> +                0, 0, 0, 0,                     \
> +                0, 0, 0, 0,                     \
> +                0, 0, 0, 0,                     \
> +                0, 0, 0, 0,                     \
> +                0, 0, 0, 0,                     \
> +                0, 0, 0, 0,                     \
> +                0, 0, 0, 0                      \
> +                } }
> +#else
> +#define __SIZEOF_VIR_BUFFER 16
> +#define VIR_BUFFER_INITIALIZER { { 0, 0, 0, 0,  \
> +                0, 0, 0, 0,                     \
> +                0, 0, 0, 0,                     \
> +                0, 0, 0, 0                      \
> +                } }
> +#endif
>  struct _virBuffer {
> -    char *content;          /* The buffer content UTF8 */
> -    unsigned int use;       /* The buffer size used */
> -    unsigned int size;      /* The buffer size */
> +    char *padding[__SIZEOF_VIR_BUFFER]; /* This struct contents is private */
>  };

  which is really not nice. 
I would prefer to relax the 'non-public' point and let the compiler 
compute the size in some ways rather than hardcode based on a word size
indication which may not take into account specific alignment problems
on some platforms.
People are lazy, if all code examples already use the proper initialization
code, then they will cut and paste. Another way is to force the initialization
macro to set up one extra field to a magic value and check it out in the
buffer methods, that would do more checking.

  Except for that minor initialization point, I like the patch a lot !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/