[Libguestfs] [PATCH] generator: Fix API of functions that return RBufferOut

[Richard W.M. Jones]

On Wed, Nov 18, 2009 at 12:17:10PM +0100, Jim Meyering wrote:
> Richard W.M. Jones wrote:
> > Subject: [PATCH] generator: Fix API of functions that return RBufferOut
> >
> > (NB: The API / ABI doesn't actually change here - it's just made much
> > simpler to use).
> >
> > The API for RBufferOut functions was unexpectedly hard to use in the
> > case where a zero-length buffer might be returned.  For discussion on
> > this see:
> ...
> > diff --git a/src/generator.ml b/src/generator.ml
> > index c5f21df..2317541 100644
> > --- a/src/generator.ml
> > +++ b/src/generator.ml
> > @@ -5107,7 +5109,7 @@ and generate_client_actions () =
> >
> >  #define error guestfs_error
> >  //#define perrorf guestfs_perrorf
> > -//#define safe_malloc guestfs_safe_malloc
> > +#define safe_malloc guestfs_safe_malloc
> >  #define safe_realloc guestfs_safe_realloc
> >  //#define safe_strdup guestfs_safe_strdup
> >  #define safe_memdup guestfs_safe_memdup
> > @@ -5396,8 +5398,20 @@ check_state (guestfs_h *g, const char *caller)
> >             pr "  /* caller will free this */\n";
> >             pr "  return safe_memdup (g, &ret.%s, sizeof (ret.%s));\n" n n
> >         | RBufferOut n ->
> > -           pr "  *size_r = ret.%s.%s_len;\n" n n;
> > -           pr "  return ret.%s.%s_val; /* caller will free */\n" n n
> > +	   pr "  /* RBufferOut is tricky: If the buffer is zero-length, then\n";
> > +	   pr "   * _val might be NULL here.  To make the API saner for\n";
> > +	   pr "   * callers, we turn this case into a unique pointer (using\n";
> > +	   pr "   * malloc(1)).\n";
> > +	   pr "   */\n";
> > +	   pr "  if (ret.%s.%s_len > 0) {\n" n n;
> > +           pr "    *size_r = ret.%s.%s_len;\n" n n;
> > +           pr "    return ret.%s.%s_val; /* caller will free */\n" n n;
> > +	   pr "  } else {\n";
> > +	   pr "    free (ret.%s.%s_val);\n" n n;
> > +	   pr "    char *p = safe_malloc (g, 1);\n";
> > +           pr "    *size_r = ret.%s.%s_len;\n" n n;
> > +           pr "    return p;\n";
> > +	   pr "  }\n";
> 
> Hi Rich,
> 
> That looks fine.
> 
> You can hoist this assignment to preced the 'if' test,
> since the same statement is in both the then and else blocks:
> 
>   *size_r = ret.%s.%s_len;\n" n n;

This was deliberate - I wanted to preserve the current behaviour which
is that *size_r isn't modified until we know we are going to exit the
function without errors.  'safe_malloc' might longjmp out of the
function.

Thanks for looking at this - I'm going to push it all now.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw