Re: [Libguestfs] [libnbd PATCH v4 05/25] golang: Change logic of copy_uint32_array
On Tue, Aug 08, 2023 at 01:15:16PM +0100, Richard W.M. Jones wrote:
On Tue, Aug 08, 2023 at 02:36:12PM +0300, Nir Soffer wrote:
> On Thu, Aug 3, 2023 at 4:57 AM Eric Blake <eblake(a)redhat.com> wrote:
> > 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\")
>
> This is unwanted in a library, it means the entire application will crash
> because of a bug in the library. Can we convert this to an error in the caller?
In Eric's defence I want to point out that this is (supposed to be) a
"should never happen" internal error, like an assert in C. What's the
proper way to handle an internal error?
Indeed - the intent here is that there is no error to return to the
caller, because the caller cannot ever set up any condition where this
will fail.
> > + }
> > 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]
>
> Can we require Go 1.17? (current version is 1.20)
>
> In Go >= 1.17, we can use something like:
>
> s := unsafe.Slice(C.uint32_t, length)
Golang 1.17 was released in Aug 2021 (2 years ago) which is still
fairly recent.
On the other hand, RHEL 8 has 1.19 for some reason -- RHEL 8 seem to
rebasing golang like crazy.
So I guess this would be fine.
> > - 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)
> > + }
>
> This loop is worse than the ugly line creating a slice.
> With a slice we can do:
>
> for i, item := range s {
> ret[i] = uint32(item)
> }
The real test is whether we can also write nice slice semantics for
the loop in 6/25, when converting an array of C structs to a Go array.
If requiring Go > 1.17 is the way to do that, then I can experiment
with requiring that as a minimum during configure.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org