Libguestfs - Jun 2019 - Re: [PATCH libnbd] generator: Add Mutable type to the generator.

Mutable (Int n) => int *n

This can currently only be used for callback arguments of type int
(not for other types, nor for any ordinary function arguments), but it
could be implemented more generally in future.
---
 generator/generator | 75 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 63 insertions(+), 12 deletions(-)

diff --git a/generator/generator b/generator/generator
index e1a97a5..72d37c8 100755
--- a/generator/generator
+++ b/generator/generator
@@ -843,6 +843,7 @@ and arg  | Flags of string          (* NBD_CMD_FLAG_* flags
*)
 | Int of string            (* small int *)
 | Int64 of string          (* 64 bit signed int *)
+| Mutable of arg           (* mutable argument, eg. int* *)
 | Opaque of string         (* opaque object, void* in C *)
 | Path of string           (* filename or path *)
 | SockAddrAndLen of string * string (* struct sockaddr * + socklen_t *)
@@ -2708,6 +2709,7 @@ let rec name_of_arg = function
 | Int n -> [n]
 | Int64 n -> [n]
 | Opaque n -> [n]
+| Mutable arg -> name_of_arg arg
 | Path n -> [n]
 | SockAddrAndLen (n, len) -> [n; len]
 | String n -> [n]
@@ -2741,6 +2743,8 @@ let rec print_c_arg_list ?(handle = false) args        |
Flags n -> pr "uint32_t %s" n
       | Int n -> pr "int %s" n
       | Int64 n -> pr "int64_t %s" n
+      | Mutable (Int n) -> pr "int *%s" n
+      | Mutable arg -> assert false
       | Opaque n -> pr "void *%s" n
       | Path n
       | String n -> pr "const char *%s" n
@@ -3261,14 +3265,20 @@ let print_python_binding name { args; ret }             
pr "  for (size_t i = 0; i < %s; ++i)\n" len;
             pr "    PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong
(%s[i]));\n" n n
          | BytesIn _ -> ()
+         | Mutable (Int n) ->
+            pr "  PyObject *py_%s_dict = PyImport_GetModuleDict
();\n" n;
+            pr "  PyObject *py_%s_mod = PyMapping_GetItemString
(py_%s_dict, \"ctypes\");\n" n n;
+            pr "  PyObject *py_%s = PyObject_CallMethod (py_%s_mod,
\"c_int\", \"i\", *%s);\n" n n n
          | Opaque n ->
             pr "  struct %s_%s_data *_data = %s;\n" name cb_name n
          | String n
          | UInt64 n -> ()
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | BytesPersistIn _ | BytesPersistOut _
+         | Callback _ | CallbackPersist _
+         | Flags _ | Int _ | Int64 _ | Mutable _
+         | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;
        pr "\n";
@@ -3278,13 +3288,16 @@ let print_python_binding name { args; ret }          
function
          | ArrayAndLen (UInt32 n, len) -> pr " \"O\""
          | BytesIn (n, len) -> pr " \"y#\""
+         | Mutable (Int n) -> pr " \"O\""
          | Opaque n -> pr " \"O\""
          | String n -> pr " \"s\""
          | UInt64 n -> pr " \"K\""
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | BytesPersistIn _ | BytesPersistOut _
+         | Callback _ | CallbackPersist _
+         | Flags _ | Int _ | Int64 _ | Mutable _
+         | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;
        pr " \")\"";
@@ -3292,13 +3305,16 @@ let print_python_binding name { args; ret }          
function
          | ArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
          | BytesIn (n, len) -> pr ", %s, (int) %s" n len
+         | Mutable (Int n) -> pr ", py_%s" n
          | Opaque _ -> pr ", _data->data"
          | String n
          | UInt64 n -> pr ", %s" n
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | BytesPersistIn _ | BytesPersistOut _
+         | Callback _ | CallbackPersist _
+         | Flags _ | Int _ | Int64 _ | Mutable _
+         | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;
        pr ");\n";
@@ -3327,14 +3343,21 @@ let print_python_binding name { args; ret }          
function
          | ArrayAndLen (UInt32 n, _) ->
             pr "  Py_DECREF (py_%s);\n" n
+         | Mutable (Int n) ->
+            pr "  PyObject *py_%s_ret = PyObject_CallMethod (py_%s,
\"value\", \"\");\n" n n;
+            pr "  *%s = PyLong_AsLong (py_%s_ret);\n" n n;
+            pr "  Py_DECREF (py_%s_ret);\n" n;
+            pr "  Py_DECREF (py_%s);\n" n
          | BytesIn _
          | String _
          | UInt64 _
          | Opaque _ -> ()
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | BytesPersistIn _ | BytesPersistOut _
+         | Callback _ | CallbackPersist _
+         | Flags _ | Int _ | Int64 _ | Mutable _
+         | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;
        pr "  return ret;\n";
@@ -3388,6 +3411,8 @@ let print_python_binding name { args; ret }      | Int64 n
->
        pr "  int64_t %s_i64;\n" n;
        pr "  long long %s; /* really int64_t */\n" n
+    | Mutable arg ->
+       pr "  PyObject *%s;\n" (List.hd (name_of_arg arg))
     | Opaque _ -> ()
     | Path n -> pr "  char *%s = NULL;\n" n
     | SockAddrAndLen (n, _) ->
@@ -3429,6 +3454,7 @@ let print_python_binding name { args; ret }      | Int _
     | Int64 _
     | Opaque _
+    | Mutable _
     | Path _
     | SockAddrAndLen _
     | String _
@@ -3452,6 +3478,7 @@ let print_python_binding name { args; ret }      | Flags n
-> pr " \"I\""
     | Int n -> pr " \"i\""
     | Int64 n -> pr " \"L\""
+    | Mutable _ -> pr " \"O\""
     | Opaque _ -> pr " \"O\""
     | Path n -> pr " \"O&\""
     | SockAddrAndLen (n, _) -> pr " \"O\""
@@ -3476,6 +3503,7 @@ let print_python_binding name { args; ret }      | Flags n
-> pr ", &%s" n
     | Int n -> pr ", &%s" n
     | Int64 n -> pr ", &%s" n
+    | Mutable arg -> pr ", &%s" (List.hd (name_of_arg arg))
     | Opaque n -> pr ", &%s_data->data" (find_callback n)
     | Path n -> pr ", PyUnicode_FSConverter, &%s" n
     | SockAddrAndLen (n, _) -> pr ", &%s" n
@@ -3525,6 +3553,8 @@ let print_python_binding name { args; ret }      | Flags n
-> pr "  %s_u32 = %s;\n" n n
     | Int _ -> ()
     | Int64 n -> pr "  %s_i64 = %s;\n" n n
+    | Mutable _ ->
+       pr "  abort (); /* Mutable for normal Python parameters not impl
*/\n"
     | Opaque n -> ()
     | Path _ -> ()
     | SockAddrAndLen _ ->
@@ -3553,6 +3583,7 @@ let print_python_binding name { args; ret }      | Flags n
-> pr ", %s_u32" n
     | Int n -> pr ", %s" n
     | Int64 n -> pr ", %s_i64" n
+    | Mutable arg -> pr ", /* XXX */ (void *) %s" (List.hd
(name_of_arg arg))
     | Opaque n -> pr ", %s_data" (find_callback n)
     | Path n -> pr ", %s" n
     | SockAddrAndLen (n, _) -> pr ", /* XXX */ (void *) %s, 0" n
@@ -3587,6 +3618,7 @@ let print_python_binding name { args; ret }      | Flags _
     | Int _
     | Int64 _
+    | Mutable _
     | Opaque _
     | Path _
     | SockAddrAndLen _
@@ -3632,6 +3664,7 @@ let print_python_binding name { args; ret }      | Flags _
-> ()
     | Int _ -> ()
     | Int64 _ -> ()
+    | Mutable _ -> ()
     | Opaque _ -> ()
     | Path n -> pr "  free (%s);\n" n
     | SockAddrAndLen _ -> ()
@@ -3730,6 +3763,7 @@ class NBD (object):
                      | Flags n -> n, Some "0"
                      | Int n -> n, None
                      | Int64 n -> n, None
+                     | Mutable arg -> List.hd (name_of_arg arg), None
                      | Opaque n -> n, None
                      | Path n -> n, None
                      | SockAddrAndLen (n, _) -> n, None
@@ -3820,6 +3854,7 @@ and ocaml_arg_to_string = function
   | OCamlArg (Flags _) -> assert false (* see above *)
   | OCamlArg (Int _) -> "int"
   | OCamlArg (Int64 _) -> "int64"
+  | OCamlArg (Mutable arg) -> ocaml_arg_to_string (OCamlArg arg) ^ "
ref"
   | OCamlArg (Opaque n) -> sprintf "'%s" n
   | OCamlArg (Path _) -> "string"
   | OCamlArg (SockAddrAndLen _) -> "string" (* XXX not impl *)
@@ -3985,13 +4020,13 @@ let print_ocaml_binding (name, { args; ret })          
List.map (
            function
            | ArrayAndLen (UInt32 n, _) | BytesIn (n, _)
-           | String n | UInt64 n | Opaque n ->
+           | String n | UInt64 n | Opaque n | Mutable (Int n) ->
               n ^ "v"
            (* The following not yet implemented for callbacks XXX *)
            | ArrayAndLen _ | Bool _ | BytesOut _
            | BytesPersistIn _ | BytesPersistOut _
            | Callback _ | CallbackPersist _
-           | Flags _ | Int _ | Int64 _ | Path _
+           | Flags _ | Int _ | Int64 _ | Path _ | Mutable _
            | SockAddrAndLen _ | StringList _
            | UInt _ | UInt32 _ -> assert false
          ) args in
@@ -4025,10 +4060,15 @@ let print_ocaml_binding (name, { args; ret })           
pr "  const struct callback_data *_%s = %s;\n" n n;
             pr "  fnv = *_%s->cb;\n" n;
             pr "  %sv = *_%s->data;\n" n n
+         | Mutable (Int n) ->
+            pr "  %sv = caml_alloc_tuple (1);\n" n;
+            pr "  Store_field (%sv, 0, Val_int (*%s));\n" n n
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
-         | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | BytesPersistIn _ | BytesPersistOut _
+         | Callback _ | CallbackPersist _
+         | Flags _ | Int _ | Int64 _ | Mutable _
+         | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;
 
@@ -4047,6 +4087,15 @@ let print_ocaml_binding (name, { args; ret })         pr
"             caml_format_exception (Extract_exception (rv)));\n";
        pr "    CAMLreturnT (int, -1);\n";
        pr "  }\n";
+
+       List.iter (
+         function
+         (* Mutable must be copied back to the C pointer. *)
+         | Mutable (Int n) ->
+            pr "  *%s = Int_val (Field (%sv, 0));\n" n n
+         | _ -> ()
+       ) args;
+
        pr "\n";
        pr "  CAMLreturnT (int, 0);\n";
        pr "}\n";
@@ -4153,6 +4202,7 @@ let print_ocaml_binding (name, { args; ret })         pr
"  int %s = Int_val (%sv);\n" n n
     | OCamlArg (Int64 n) ->
        pr "  int64_t %s = Int64_val (%sv);\n" n n
+    | OCamlArg (Mutable _) -> assert false
     | OCamlArg (Opaque n) ->
        (match find_callback n with
         | Callback (cb_name, _) ->
@@ -4252,6 +4302,7 @@ let print_ocaml_binding (name, { args; ret })      |
OCamlArg (Flags _)
     | OCamlArg (Int _)
     | OCamlArg (Int64 _)
+    | OCamlArg (Mutable _)
     | OCamlArg (Opaque _)
     | OCamlArg (Path _)
     | OCamlArg (String _)
-- 
2.22.0

On 6/25/19 12:11 PM, Richard W.M. Jones wrote:
> Mutable (Int n) => int *n
> 
> This can currently only be used for callback arguments of type int
> (not for other types, nor for any ordinary function arguments), but it
> could be implemented more generally in future.
> ---
>  generator/generator | 75 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 63 insertions(+), 12 deletions(-)
Looks good to me.

As mentioned in other threads [1], I'm interested in attempting some
sort of:

nbd_aio_*_callback (other args, data, callback, flags)

for all transmission requests (or name it nbd_aio_*_with_notify, or
whatever other name we come up with), which calls the callback the
moment that particular reply from the server is ready. The ideal
callback signature would thus be:

int callback(void *opaque, int64_t handle, int *status)

where status on input is what the server replied, and on output is what
the overall nbd_aio_command_completed() will report.

In the nbdkit case, I'm hoping that the callback can change things -
right now, nbdkit uses the I/O polling thread to call
nbd_aio_command_completed with an nbdkit lock held, and for each
completed command, raise the semaphore to let the nbdkit thread that
issued the request wake back up. But with the callback, the I/O polling
thread doesn't ever have to grab an nbdkit lock - the callback would be
the one releasing the semaphore, then the nbdkit thread that made the
request would be the one also calling nbd_aio_command_completed.

Also, in the case of nbd_aio_pread_structured_callback(), it also
creates the nice setup of:

struct pread_validator *data generate_pread_structured_validator(offset, length,
flags);
handle = nbd_aio_pread_structured_callback(h, buf, count, offset, data,
validate_chunk, validate_complete, flags);

so that validate_complete() can call the appropriate free(data) AND do
any final error manipulations (change a successful return into a failure
if there were insufficient calls to validate_chunk to cover the entire
buffer).


[1] https://www.redhat.com/archives/libguestfs/2019-June/msg00163.html

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

On 6/25/19 12:11 PM, Richard W.M. Jones wrote:
> Mutable (Int n) => int *n
> 
> This can currently only be used for callback arguments of type int
> (not for other types, nor for any ordinary function arguments), but it
> could be implemented more generally in future.
> ---
>  generator/generator | 75 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 63 insertions(+), 12 deletions(-)
> 
> @@ -3261,14 +3265,20 @@ let print_python_binding name { args; ret } >   
pr "  for (size_t i = 0; i < %s; ++i)\n" len;
>              pr "    PyList_SET_ITEM (py_%s, i,
PyLong_FromUnsignedLong (%s[i]));\n" n n
>           | BytesIn _ -> ()
> +         | Mutable (Int n) ->
> +            pr "  PyObject *py_%s_dict = PyImport_GetModuleDict
();\n" n;
> +            pr "  PyObject *py_%s_mod = PyMapping_GetItemString
(py_%s_dict, \"ctypes\");\n" n n;
> +            pr "  PyObject *py_%s = PyObject_CallMethod (py_%s_mod,
\"c_int\", \"i\", *%s);\n" n n n
PyMapping_GetItemString() is returning NULL, then the program is
segfaulting on PyObject_CallMethod. This code needs some error checking
to be safe, as well as a tweak to more properly call into Python (did we
forget an earlier global import of the ctypes module?).

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

On 6/25/19 3:47 PM, Eric Blake wrote:
> On 6/25/19 12:11 PM, Richard W.M. Jones wrote:
>> Mutable (Int n) => int *n
>>
>> This can currently only be used for callback arguments of type int
>> (not for other types, nor for any ordinary function arguments), but it
>> could be implemented more generally in future.
>> ---
>>  generator/generator | 75 +++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 63 insertions(+), 12 deletions(-)
>>
> 
>> @@ -3261,14 +3265,20 @@ let print_python_binding name { args; ret }
>>              pr "  for (size_t i = 0; i < %s; ++i)\n" len;
>>              pr "    PyList_SET_ITEM (py_%s, i,
PyLong_FromUnsignedLong (%s[i]));\n" n n
>>           | BytesIn _ -> ()
>> +         | Mutable (Int n) ->
>> +            pr "  PyObject *py_%s_dict = PyImport_GetModuleDict
();\n" n;
>> +            pr "  PyObject *py_%s_mod = PyMapping_GetItemString
(py_%s_dict, \"ctypes\");\n" n n;
>> +            pr "  PyObject *py_%s = PyObject_CallMethod
(py_%s_mod, \"c_int\", \"i\", *%s);\n" n n n
> 
> PyMapping_GetItemString() is returning NULL, then the program is
> segfaulting on PyObject_CallMethod. This code needs some error checking
> to be safe, as well as a tweak to more properly call into Python (did we
> forget an earlier global import of the ctypes module?).
> 
Here's what I squashed in to make it work in Python 3 (if we want to
support Python 2, we also need a configure check for
PyString_From_String and use that instead - but I'm of the opinion that
python 2 is close enough to death that libnbd need not worry about it,
even if nbdkit still supports it).

diff --git i/generator/generator w/generator/generator
index fadbdfc..b5c4e9a 100755
--- i/generator/generator
+++ w/generator/generator
@@ -3380,9 +3380,13 @@ let print_python_binding name { args; ret }           |
BytesIn _
          | Int _ -> ()
          | Mutable (Int n) ->
-            pr "  PyObject *py_%s_dict = PyImport_GetModuleDict
();\n" n;
-            pr "  PyObject *py_%s_mod = PyMapping_GetItemString
(py_%s_dict, \"ctypes\");\n" n n;
-            pr "  PyObject *py_%s = PyObject_CallMethod (py_%s_mod,
\"c_int\", \"i\", *%s);\n" n n n
+            pr "  PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
+            pr "  if (!py_%s_modname) { PyErr_PrintEx (0); return -1;
}\n" n;
+            pr "  PyObject *py_%s_mod = PyImport_Import
(py_%s_modname);\n" n n;
+            pr "  Py_DECREF (py_%s_modname);\n" n;
+            pr "  if (!py_%s_mod) { PyErr_PrintEx (0); return -1;
}\n" n;
+            pr "  PyObject *py_%s = PyObject_CallMethod (py_%s_mod,
\"c_int\", \"i\", *%s);\n" n n n;
+            pr "  if (!py_%s) { PyErr_PrintEx (0); return -1; }\n" n;
          | Opaque n ->
             pr "  struct %s_%s_data *_data = %s;\n" name cb_name n
          | String n
@@ -3460,7 +3464,7 @@ let print_python_binding name { args; ret }           |
ArrayAndLen (UInt32 n, _) ->
             pr "  Py_DECREF (py_%s);\n" n
          | Mutable (Int n) ->
-            pr "  PyObject *py_%s_ret = PyObject_CallMethod (py_%s,
\"value\", \"\");\n" n n;
+            pr "  PyObject *py_%s_ret = PyObject_GetAttrString (py_%s,
\"value\");\n" n n;
             pr "  *%s = PyLong_AsLong (py_%s_ret);\n" n n;
             pr "  Py_DECREF (py_%s_ret);\n" n;
             pr "  Py_DECREF (py_%s);\n" n


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