Re: [Libguestfs] [libnbd PATCH v4 05/25] golang: Change logic of copy_uint32_array

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 <lersek(a)redhat.com&gt; > CC: Nir Soffer <nsoffer(a)redhat.com&gt; > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > --- > > 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 <rjones(a)redhat.com&gt;