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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/
-------------- next part --------------
>From f5a3da14837fe65eab1df41df33f2bafeddcd9ce Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Wed, 18 Nov 2009 10:37:10 +0000
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:

https://www.redhat.com/archives/libguestfs/2009-November/thread.html#00115

This commit ensures that in the zero-length buffer case, the return
value is never NULL.  Thus code is now able to just check if the return
value == NULL to indicate an error, which is simpler for all concerned.

The implementation of this is, however, more complex because we have
to be careful about this case inside both the daemon and the library
code, which is what this commit does.

This has passed a full round of tests.
---
 fuse/guestmount.c |    8 +-------
 src/generator.ml  |   46 +++++++++++++++++++++++++++++++++++++---------
 2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/fuse/guestmount.c b/fuse/guestmount.c
index ba0d626..baf2b66 100644
--- a/fuse/guestmount.c
+++ b/fuse/guestmount.c
@@ -620,14 +620,8 @@ fg_read (const char *path, char *buf, size_t size, off_t
offset,
   if (size > limit)
     size = limit;
 
-  /* Note the correct error handling here is tricky, because in the
-   * case where the call returns a zero-length buffer, it might return
-   * NULL.  However it won't adjust rsize along the error path, so we
-   * can set rsize to something beforehand and use that as a flag.
-   */
-  rsize = 1;
   r = guestfs_pread (g, path, size, offset, &rsize);
-  if (rsize == 1 && r == NULL)
+  if (r == NULL)
     return error ();
 
   /* This should never happen, but at least it stops us overflowing
diff --git a/src/generator.ml b/src/generator.ml
index c5f21df..2317541 100644
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -3890,7 +3890,9 @@ into smaller groups of names.");
 
   ("pread", (RBufferOut "content", [Pathname
"path"; Int "count"; Int64 "offset"]), 207,
[ProtocolLimitWarning],
    [InitISOFS, Always, TestOutputBuffer (
-      [["pread"; "/known-4"; "1";
"3"]], "\n")],
+      [["pread"; "/known-4"; "1";
"3"]], "\n");
+    InitISOFS, Always, TestOutputBuffer (
+      [["pread"; "/empty"; "0";
"100"]], "")],
    "read part of a file",
    "\
 This command lets you read part of a file.  It reads C<count>
@@ -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";
       );
 
       pr "}\n\n"
@@ -5480,7 +5494,7 @@ and generate_daemon_actions ()          | RStruct (_, typ)
-> pr "  guestfs_int_%s *r;\n" typ; "NULL"
         | RStructList (_, typ) -> pr "  guestfs_int_%s_list *r;\n"
typ; "NULL"
         | RBufferOut _ ->
-            pr "  size_t size;\n";
+            pr "  size_t size = 1;\n";
             pr "  char *r;\n";
             "NULL" in
 
@@ -5573,10 +5587,24 @@ and generate_daemon_actions ()       
generate_c_call_args (fst style, args');
       pr ";\n";
 
-      pr "  if (r == %s)\n" error_code;
-      pr "    /* do_%s has already called reply_with_error */\n"
name;
-      pr "    goto done;\n";
-      pr "\n";
+      (match fst style with
+       | RErr | RInt _ | RInt64 _ | RBool _
+       | RConstString _ | RConstOptString _
+       | RString _ | RStringList _ | RHashtable _
+       | RStruct (_, _) | RStructList (_, _) ->
+	   pr "  if (r == %s)\n" error_code;
+	   pr "    /* do_%s has already called reply_with_error */\n" name;
+	   pr "    goto done;\n";
+	   pr "\n"
+       | RBufferOut _ ->
+	   pr "  /* size == 0 && r == NULL could be a non-error case
(just\n";
+	   pr "   * an ordinary zero-length buffer), so be careful ...\n";
+	   pr "   */\n";
+	   pr "  if (size == 1 && r == %s)\n" error_code;
+	   pr "    /* do_%s has already called reply_with_error */\n" name;
+	   pr "    goto done;\n";
+	   pr "\n"
+      );
 
       (* If there are any FileOut parameters, then the impl must
        * send its own reply.
-- 
1.6.5.2

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;

Also, note the mix of indentatation spaces and TABs in added lines.