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.

diff --git a/fuse/guestmount.c b/fuse/guestmount.c
index ba0d626..baf2b66 100644 (file)
--- 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;
 
   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);
   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
     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 (file)
--- 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", (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>
    "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 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
 #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 "  /* 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"
       );
 
       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 _ ->
         | 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
 
             pr "  char *r;\n";
             "NULL" in
 


@@ -5573,10 +5587,24 @@ and generate_daemon_actions () =
       generate_c_call_args (fst style, args');
       pr ";\n";
 
       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.
 
       (* If there are any FileOut parameters, then the impl must
        * send its own reply.