Re: [Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation

On 09/20/21 17:30, Daniel P. Berrangé wrote: > On Mon, Sep 20, 2021 at 05:23:30PM +0200, Laszlo Ersek wrote: >> On 09/20/21 14:33, Daniel P. Berrangé wrote: >>> On Mon, Sep 20, 2021 at 12:03:51PM +0100, Richard W.M. Jones wrote: >>>> On Mon, Sep 20, 2021 at 11:37:02AM +0100, Daniel P. Berrangé wrote: >>>>> What distro / go version do you see this on, as I can't reproduce >>>>> this pointer problem with a standalone demo app ? >>>> >>>> For me this started to happen after upgrading to >>>> golang-bin-1.17-2.fc36.x86_64 in Rawhide. It also caused this error: >>> >>> Hmm, I still cant reproduce the problem that Laszlo is fixing >>> >>> $ cat str.c >>> >>> #include <stdio.h> >>> >>> void foo(char **str) { >>> for (int i = 0; str[i] != NULL; i++) { >>> fprintf(stderr, "%d: %s (0x%p)\n", i, str[i], str[i]); >>> } >>> } >>> >>> $ cat str.go >>> package main >>> >>> /* >>> #cgo LDFLAGS: -L/home/berrange/t/lib -lstr >>> >>> #include <stdlib.h> >>> >>> extern void foo(char **str); >>> >>> */ >>> import "C" >>> >>> import ( >>> "fmt" >>> "unsafe" >>> ) >>> >>> func array_elem(arr **C.char, idx int) **C.char { >>> return (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(arr)) + >>> (unsafe.Sizeof(arr) * uintptr(idx)))) >>> } >>> >>> func arg_string_list1(xs []string) **C.char { >>> r := make([]*C.char, 1+len(xs)) >>> for i, x := range xs { >>> r[i] = C.CString(x) >>> } >>> r[len(xs)] = nil >>> return &r[0] >>> } >>> >>> func arg_string_list2(xs []string) **C.char { >>> var r **C.char >>> r = (**C.char)(C.malloc(C.size_t(unsafe.Sizeof(*r) * (1 + uintptr(len(xs)))))) >>> for i, x := range xs { >>> str := array_elem(r, i) >>> *str = C.CString(x) >>> } >>> str := array_elem(r, len(xs)) >>> *str = nil >>> return r >>> } >>> >>> func free_string_list(argv **C.char) { >>> for i := 0; ; i++ { >>> str := (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(argv)) + >>> (unsafe.Sizeof(*argv) * uintptr(i)))) >>> if *str == nil { >>> break >>> } >>> fmt.Printf("%x\n", *str) >>> C.free(unsafe.Pointer(*str)) >>> } >>> } >>> >>> func bar(str []string) { >>> cstr1 := arg_string_list1(str) >>> defer free_string_list(cstr1) >>> C.foo(cstr1) >>> cstr2 := arg_string_list2(str) >>> defer free_string_list(cstr2) >>> C.foo(cstr2) >>> } >>> >>> func main() { >>> bar([]string{"hello", "world"}) >>> } >>> >>> >>> My interpretation is that arg_string_list1 impl here should have >>> raised the error that Laszlo reports, but both impls work fine >> >> Can you create a new structure type, make the C function take the structure (or a pointer to the structure), and in the structure, make the field have this type: >> >> char * const * str; >> >> Because this is the scenario where the libguestfs test suite fails (panics). The libguestfs test suite has a *different* case that does match your example directly, and *that* case works in the libguestfs test suite flawlessly. The panic surfaces only in the "char*const* embedded in struct" case. (I assume "const" makes no difference, but who knows!) > > Oh, that makes sense, because you have a Go pointer to the storage for > the struct, and then the 'const *const *str' field is initialized with > a Go pointer returned from the arg_string_list(). > > You're allowed to pass a Go pointer to C via CGo, but the memory that > points to is not allowed to contained further Go pointers. So the struct > fields must strictly use a C pointer. If I take your last paragraph here and work it into the commit message / comment of this patch, will you accept the code in the patch? I really don't insist on getting *this* particular patch in. What I'd like to achieve is a clean "make check" baseline, so I can run the test suite regularly, as I get into fixing other libguestfs BZs. I don't intend to "maintain" Go bindings; I consider this a one-off fix. I'm uncomfortable contributing any Go code that's not as "thin" as I can possibly manage. (And honestly I think the swaths of "explicit unsafe" just make things unreadable.) So I could do two things: - push patches #1-#3 and drop #4, allowing someone else to fix the issue described in #4 in more idiomatic Go, - post a v2 of #4 with updated comments / commit message. I think a "less idiomatic but technically correct" Go binding is still an improvement over a panicking Go runtime, but again, if someone can fix this more idiomatically, I'll *gladly* defer to them. Rich, what's your take?