Libguestfs - Nov 2021 - [libnbd PATCH] golang: Simplify nbd_block_status callback array

Instead of copying from a C uint32_t[] to a golang []uint32, we can
exploit the fact that their underlying memory has the same
representation.  An unsafe cast to more memory than necessary exposes
all the more that Go then needs to give a well-bounded slice, with no
copying necessary.

https://newbedev.com/pass-struct-and-array-of-structs-to-c-function-from-go
---
 generator/GoLang.ml | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/generator/GoLang.ml b/generator/GoLang.ml
index eb3aa26..7455dde 100644
--- a/generator/GoLang.ml
+++ b/generator/GoLang.ml
@@ -1,6 +1,6 @@
 (* hey emacs, this is OCaml code: -*- tuareg -*- *)
 (* nbd client library in userspace: generator
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2021 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -513,13 +513,14 @@ import \"unsafe\"

 /* Closures. */

-func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
-    ret := make([]uint32, int (count))
-    for i := 0; i < int (count); i++ {
-       entry := (*C.uint32_t) (unsafe.Pointer(uintptr(unsafe.Pointer(entries))
+ (unsafe.Sizeof(*entries) * uintptr(i))))
-       ret[i] = uint32 (*entry)
-    }
-    return ret
+func use_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
+    /*
https://stackoverflow.com/questions/48756732/what-does-1-30c-yourtype-do-exactly-in-cgo
*/
+    unsafePtr := unsafe.Pointer(entries)
+    /* Max structured reply payload is 64M, so this array size is more than
+     * sufficient for the underlying slice we want to expose.
+     */
+    arrayPtr := (*[1 << 20]uint32)(unsafePtr)
+    return arrayPtr[:count:count]
 }
 ";

@@ -601,7 +602,7 @@ func copy_uint32_array (entries *C.uint32_t, count C.size_t)
[]uint32 {
           if !comma then pr ", "; comma := true;
           match cbarg with
           | CBArrayAndLen (UInt32 n, count) ->
-             pr "copy_uint32_array (%s, %s)" n count
+             pr "use_uint32_array (%s, %s)" n count
           | CBBytesIn (n, len) ->
              pr "C.GoBytes (%s, C.int (%s))" n len
           | CBInt n ->
-- 
2.33.1

On Wed, Nov 24, 2021 at 10:03 PM Eric Blake <eblake at redhat.com>
wrote:
>
> Instead of copying from a C uint32_t[] to a golang []uint32, we can
> exploit the fact that their underlying memory has the same
> representation.  An unsafe cast to more memory than necessary exposes
> all the more that Go then needs to give a well-bounded slice, with no
> copying necessary.
Nice addition, I did know that we do this extra unneeded copy, and it will be
nice to avoid it.

But do we document that the caller must copy the returned entries
array if they need
to keep it after the extent callback returns?

This change may cause use after free in caller code if the code is not
careful about
that. I think a warning in the ExtentCallback wrapper would be useful.

This is the documentation we have now:

    https://pkg.go.dev/libguestfs.org/libnbd#ExtentCallback
>
> https://newbedev.com/pass-struct-and-array-of-structs-to-c-function-from-go
Maybe a better link is:
https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices

It also explains a nicer way with Go 1.17 which we can use in a future release.
> ---
>  generator/GoLang.ml | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
> index eb3aa26..7455dde 100644
> --- a/generator/GoLang.ml
> +++ b/generator/GoLang.ml
> @@ -1,6 +1,6 @@
>  (* hey emacs, this is OCaml code: -*- tuareg -*- *)
>  (* nbd client library in userspace: generator
> - * Copyright (C) 2013-2020 Red Hat Inc.
> + * Copyright (C) 2013-2021 Red Hat Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -513,13 +513,14 @@ import \"unsafe\"
>
>  /* Closures. */
>
> -func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
> -    ret := make([]uint32, int (count))
> -    for i := 0; i < int (count); i++ {
> -       entry := (*C.uint32_t)
(unsafe.Pointer(uintptr(unsafe.Pointer(entries)) + (unsafe.Sizeof(*entries) *
uintptr(i))))
> -       ret[i] = uint32 (*entry)
> -    }
> -    return ret
> +func use_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {
> +    /*
https://stackoverflow.com/questions/48756732/what-does-1-30c-yourtype-do-exactly-in-cgo
*/
> +    unsafePtr := unsafe.Pointer(entries)
> +    /* Max structured reply payload is 64M, so this array size is more
than
> +     * sufficient for the underlying slice we want to expose.
> +     */
Why 64M?

I guess you calculate using maximum request length (2**31 - 1) and
minimum block size (512)
so we can get up to 8388607 extents per request, with 8 bytes per
extent - 67108856 bytes.
If this is the reason, maybe we should include it in
https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md
> +    arrayPtr := (*[1 << 20]uint32)(unsafePtr)
> +    return arrayPtr[:count:count]
>  }
>  ";
>
> @@ -601,7 +602,7 @@ func copy_uint32_array (entries *C.uint32_t, count
C.size_t) []uint32 {
This function is called copy_...
>            if !comma then pr ", "; comma := true;
>            match cbarg with
>            | CBArrayAndLen (UInt32 n, count) ->
> -             pr "copy_uint32_array (%s, %s)" n count
> +             pr "use_uint32_array (%s, %s)" n count
But we implement by use_... - this may confuse someone in the future,
and lead to use after free.
>            | CBBytesIn (n, len) ->
>               pr "C.GoBytes (%s, C.int (%s))" n len
>            | CBInt n ->
> --
> 2.33.1
Otherwise it looks good to me.

Nir