On Wed, Aug 02, 2023 at 08:50:25PM -0500, Eric Blake wrote:
> Commit 6725fa0e12 changed copy_uint32_array() to utilize a Go hack for
> accessing a C array as a Go slice in order to potentially benefit from
> any optimizations in Go's copy() for bulk transfer of memory over
> naive one-at-a-time iteration. But that commit also acknowledged that
> no benchmark timings were performed, which would have been useful to
> demonstrat an actual benefit for using hack in the first place. And
"demonstrate"
> since we are copying data anyways (rather than using the slice to
> avoid a copy), and network transmission costs have a higher impact to
> performance than in-memory copying speed, it's hard to justify keeping
> the hack without hard data.
>
> What's more, while using Go's copy() on an array of C uint32_t makes
> sense for 32-bit extents, our corresponding 64-bit code uses a struct
> which does not map as nicely to Go's copy(). Using a common style
> between both list copying helpers is beneficial to mainenance.
>
> Additionally, at face value, converting C.size_t to int may truncate;
> we could avoid that risk if we were to uniformly use uint64 instead of
> int. But we can equally just panic if the count is oversized: our
> state machine guarantees that the server's response fits within 64M
> bytes (count will be smaller than that, since it is multiple bytes per
> extent entry).
>
> Suggested-by: Laszlo Ersek <ler...@redhat.com>
> CC: Nir Soffer <nsof...@redhat.com>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
>
> v4: new patch to the series, but previously posted as part of the
> golang cleanups. Since then: rework the commit message as it is no
> longer a true revert, and add a panic() if count exceeds expected
> bounds.
> ---
> generator/GoLang.ml | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/generator/GoLang.ml b/generator/GoLang.ml
> index 73df5254..cc7d78b6 100644
> --- a/generator/GoLang.ml
> +++ b/generator/GoLang.ml
> @@ -516,11 +516,16 @@ let
> /* Closures. */
>
> func copy_uint32_array(entries *C.uint32_t, count C.size_t) []uint32 {
> + if (uint64(count) > 64*1024*1024) {
> + panic(\"violation of state machine guarantee\")
> + }
Not sure if golang includes the line number when it panics, but a
brief bit of explanation might help here. Something like:
panic(\"internal error: state machine should guarantee <= 64M of data \
in the extents reply, but this was exceeded unexpectedly\")
> ret := make([]uint32, int(count))
> - // See
> https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices
> - // TODO: Use unsafe.Slice() when we require Go 1.17.
> - s := (*[1 << 30]uint32)(unsafe.Pointer(entries))[:count:count]
The whole magical 1 << 30 was another thing to dislike about golang,
so glad to see it gone here.
> - copy(ret, s)
> + addr := uintptr(unsafe.Pointer(entries))
> + for i := 0; i < int(count); i++ {
> + ptr := (*C.uint32_t)(unsafe.Pointer(addr))
> + ret[i] = uint32(*ptr)
> + addr += unsafe.Sizeof(*ptr)
> + }
> return ret
> }
Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
