Libguestfs - Jul 2019 - [libnbd PATCH] generator: Let nbd_aio_get_direction return unsigned

The return value is a bitmask, and is already documented as never
failing, hence it makes sense to return an unsigned value to make it
easier to do bitwise operations without worrying about potential
undefined C semantics on a signed value.
---

I'm not sure if we want this, or even if we do if my OCaml was the
most concise, but it matches the sentiment of our recent effort to let
the valid_flag and status parameters to callbacks be unsigned.

 generator/generator | 93 +++++++++++++++++++++++++++++----------------
 lib/is-state.c      |  2 +-
 2 files changed, 61 insertions(+), 34 deletions(-)

diff --git a/generator/generator b/generator/generator
index 2cd83f1..bb3a1b5 100755
--- a/generator/generator
+++ b/generator/generator
@@ -873,6 +873,7 @@ and ret  | RInt                     (* return a small int,
-1 = error *)
 | RInt64                   (* 64 bit int, -1 = error *)
 | RString                  (* return a newly allocated string, caller frees *)
+| RUInt                    (* return a bitmask, no error possible *)
 and permitted_state  | Created                  (* can be called in the START
state *)
 | Connecting               (* can be called when connecting/handshaking *)
@@ -2013,7 +2014,7 @@ Do not do anything else with the file descriptor.";

   "aio_get_direction", {
     default_call with
-    args = []; ret = RInt; is_locked = false; may_set_error = false;
+    args = []; ret = RUInt; is_locked = false; may_set_error = false;
     shortdesc = "return the read or write direction";
     longdesc = "\
 Return the current direction of this connection, which means
@@ -3040,13 +3041,15 @@ let () 
   (* !may_set_error is incompatible with permitted_states != [] because
    * an incorrect state will result in set_error being called by the
-   * generated wrapper.
+   * generated wrapper.  It is also incompatible with RUint.
    *)
   List.iter (
     function
     | name, { permitted_states = (_::_); may_set_error = false } ->
        failwithf "%s: if may_set_error is false, permitted_states must be
empty (any permitted state)"
                  name
+    | name, { ret = RUInt; may_set_error = true } ->
+       failwithf "%s: if ret is RUInt, may_set_error must be false"
name
     | _ -> ()
   ) handle_calls

@@ -3189,6 +3192,7 @@ let print_call name args ret     | RConstString -> pr
"const char *"
    | RInt64 -> pr "int64_t "
    | RString -> pr "char *"
+   | RUInt -> pr "unsigned "
   );
   pr "nbd_%s " name;
   print_arg_list ~handle:true args
@@ -3329,10 +3333,11 @@ let generate_lib_api_c ()        | RBool
       | RErr
       | RFd
-      | RInt -> "int", "-1"
-      | RConstString -> "const char *", "NULL"
-      | RInt64 -> "int64_t", "-1"
-      | RString -> "char *", "NULL" in
+      | RInt -> "int", Some "-1"
+      | RConstString -> "const char *", Some "NULL"
+      | RInt64 -> "int64_t", Some "-1"
+      | RString -> "char *", Some "NULL"
+      | RUInt -> "unsigned", None in

     pr "%s\n" ret_c_type;
     pr "nbd_%s " name;
@@ -3347,6 +3352,7 @@ let generate_lib_api_c ()       | RConstString -> pr
"  const char *ret;\n"
      | RInt64 -> pr "  int64_t ret;\n"
      | RString -> pr "  char *ret;\n"
+     | RUInt -> pr "  unsigned ret;\n"
     );
     pr "\n";
     if may_set_error then (
@@ -3356,8 +3362,11 @@ let generate_lib_api_c ()      if is_locked then
       pr "  pthread_mutex_lock (&h->lock);\n";
     if permitted_states <> [] then (
+      let value = match errcode with
+        | Some value -> value
+        | None -> assert false in
       pr "  if (!%s_in_permitted_state (h)) {\n" name;
-      pr "    ret = %s;\n" errcode;
+      pr "    ret = %s;\n" value;
       pr "    goto out;\n";
       pr "  }\n"
     );
@@ -3412,31 +3421,37 @@ let print_api (name, { args; ret; permitted_states;
shortdesc; longdesc;
     match ret with
     | RBool ->
        pr "This call returns a boolean value.\n";
-       "-1"
+       Some "-1"
    | RConstString ->
        pr "This call returns a statically allocated string.\n";
        pr "You B<must not> try to free the string.\n";
-       "NULL"
+       Some "NULL"
    | RErr ->
        pr "If the call is successful the function returns
C<0>.\n";
-       "-1"
+       Some "-1"
    | RFd ->
        pr "This call returns a file descriptor.\n";
-       "-1"
+       Some "-1"
    | RInt ->
        pr "This call returns an integer E<ge> 0.\n";
-       "-1"
+       Some "-1"
    | RInt64 ->
        pr "This call returns a 64 bit signed integer E<ge>
0.\n";
-       "-1"
+       Some "-1"
    | RString ->
        pr "This call returns a string.  The caller must free the\n";
        pr "returned string to avoid a memory leak.\n";
-       "NULL"
+       Some "NULL"
+   | RUInt ->
+       pr "This call returns a bitmask.\n";
+       None
   in
   pr "\n";
   if may_set_error then (
-    pr "On error C<%s> is returned.\n" errcode;
+    let value = match errcode with
+      | Some value -> value
+      | None -> assert false in
+    pr "On error C<%s> is returned.\n" value;
     pr "See L<libnbd(3)/ERROR HANDLING> for how to get further
details\n";
     pr "of the error.\n"
   )
@@ -3683,7 +3698,7 @@ PyInit_libnbdmod (void)
 }
 "

-let print_python_binding name { args; ret } +let print_python_binding name {
args; ret; may_set_error }    (* Functions with a Closure parameter are special
because we
    * have to generate wrapper functions which translate the
    * callbacks back to Python.
@@ -3835,6 +3850,7 @@ let print_python_binding name { args; ret }     |
RConstString -> pr "  const char *ret;\n"
    | RInt64 -> pr "  int64_t ret;\n"
    | RString -> pr "  char *ret;\n";
+   | RUInt -> pr "  unsigned ret;\n"
   );
   pr "  PyObject *py_ret;\n";
   List.iter (
@@ -4018,14 +4034,17 @@ let print_python_binding name { args; ret }      |
UInt64 n -> pr ", %s_u64" n
   ) args;
   pr ");\n";
-  (match ret with
-   | RBool | RErr | RFd | RInt | RInt64 -> pr "  if (ret == -1)
{\n";
-   | RConstString | RString -> pr "  if (ret == NULL) {\n";
+  if may_set_error then (
+    (match ret with
+     | RBool | RErr | RFd | RInt | RInt64 -> pr "  if (ret == -1)
{\n";
+     | RConstString | RString -> pr "  if (ret == NULL) {\n";
+     | RUInt -> assert false
+    );
+    pr "    raise_exception ();\n";
+    pr "    py_ret = NULL;\n";
+    pr "    goto out;\n";
+    pr "  }\n"
   );
-  pr "    raise_exception ();\n";
-  pr "    py_ret = NULL;\n";
-  pr "    goto out;\n";
-  pr "  }\n";

   (* Convert the result back to a Python object and return it. *)
   let use_ret = ref true in
@@ -4062,7 +4081,8 @@ let print_python_binding name { args; ret }         pr
"  py_ret = Py_None;\n";
        pr "  Py_INCREF (py_ret);\n"
     | RFd
-    | RInt ->
+    | RInt
+    | RUInt ->
        pr "  py_ret = PyLong_FromLong (ret);\n"
     | RInt64 ->
        pr "  py_ret = PyLong_FromLongLong (ret);\n"
@@ -4072,7 +4092,9 @@ let print_python_binding name { args; ret }    );

   pr "\n";
-  pr " out:\n";
+  if may_set_error then (
+    pr " out:\n"
+  );
   List.iter (
     function
     | ArrayAndLen (UInt32 n, len) -> pr "  free (%s);\n" n
@@ -4347,6 +4369,7 @@ and ocaml_ret_to_string = function
   | RInt -> "int"
   | RInt64 -> "int64"
   | RString -> "string"
+  | RUInt -> "int"

 let rec name_of_ocaml_arg = function
   | OCamlHandle -> "h"
@@ -4747,9 +4770,10 @@ let print_ocaml_binding (name, { args; ret }) 
   let errcode      match ret with
-    | RBool | RErr | RFd | RInt | RInt64 -> pr "  int r;\n";
"-1"
-    | RConstString -> pr "  const char *r;\n"; "NULL"
-    | RString -> pr "  char *r;\n"; "NULL" in
+    | RBool | RErr | RFd | RInt | RInt64 -> pr "  int r;\n"; Some
"-1"
+    | RConstString -> pr "  const char *r;\n"; Some
"NULL"
+    | RString -> pr "  char *r;\n"; Some "NULL"
+    | RUInt -> pr "  unsigned r;\n"; None in
   pr "\n";
   pr "  caml_enter_blocking_section ();\n";
   pr "  r =  nbd_%s " name;
@@ -4757,14 +4781,17 @@ let print_ocaml_binding (name, { args; ret })    pr
";\n";
   pr "  caml_leave_blocking_section ();\n";
   pr "\n";
-  pr "  if (r == %s)\n" errcode;
-  pr "    nbd_internal_ocaml_raise_error ();\n";
-  pr "\n";
+  (match errcode with
+   | Some code ->
+      pr "  if (r == %s)\n" code;
+      pr "    nbd_internal_ocaml_raise_error ();\n";
+      pr "\n"
+   | None -> ()
+  );
   (match ret with
    | RBool -> pr "  rv = Val_bool (r);\n"
    | RErr -> pr "  rv = Val_unit;\n"
-   | RFd -> pr "  rv = Val_int (r);\n"
-   | RInt -> pr "  rv = Val_int (r);\n"
+   | RFd | RInt | RUInt -> pr "  rv = Val_int (r);\n"
    | RInt64 -> pr "  rv = caml_copy_int64 (r);\n"
    | RConstString -> pr "  rv = caml_copy_string (r);\n"
    | RString ->
diff --git a/lib/is-state.c b/lib/is-state.c
index d3335fb..1a85c7a 100644
--- a/lib/is-state.c
+++ b/lib/is-state.c
@@ -144,7 +144,7 @@ nbd_unlocked_aio_is_closed (struct nbd_handle *h)
   return nbd_internal_is_state_closed (get_public_state (h));
 }

-int
+unsigned
 nbd_unlocked_aio_get_direction (struct nbd_handle *h)
 {
   return nbd_internal_aio_get_direction (get_public_state (h));
-- 
2.20.1

On Thu, Jul 25, 2019 at 04:11:42PM -0500, Eric Blake
wrote:
> @@ -4072,7 +4092,9 @@ let print_python_binding name { args; ret } >   
);
> 
>    pr "\n";
> -  pr " out:\n";
> +  if may_set_error then (
> +    pr " out:\n"
> +  );
As a point merely of style, you don't need the ( .. ) around the
then-clause if you don't want it (but in that case the pr command must
be followed by a semicolon).
> @@ -4347,6 +4369,7 @@ and ocaml_ret_to_string = function
>    | RInt -> "int"
>    | RInt64 -> "int64"
>    | RString -> "string"
> +  | RUInt -> "int"
New OCaml actually has unsigned types, but we probably don't want to
depend on that version yet so this is OK as it is.

This patch looks fine, can't see any problems, so ACK.

There are a handful of places in tests and examples where we assign
nbd_aio_get_direction to an int variable, usually called ’dir’.  Maybe
we should replace those with unsigned variables?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 7/25/19 4:11 PM, Eric Blake wrote:
> The return value is a bitmask, and is already documented as never
> failing, hence it makes sense to return an unsigned value to make it
> easier to do bitwise operations without worrying about potential
> undefined C semantics on a signed value.
> ---
> 
> I'm not sure if we want this, or even if we do if my OCaml was the
> most concise, but it matches the sentiment of our recent effort to let
> the valid_flag and status parameters to callbacks be unsigned.
By the way, this changes API but not ABI; and in a way that most users
won't notice (thanks to C's looseness on implicit type conversions, an
ABI-compatible change in return type tends to not cause compile errors
unless you were using function pointers that force a bit more type safety).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org