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

On 09/20/21 12:37, Daniel P. Berrangé wrote: > On Mon, Sep 20, 2021 at 07:23:35AM +0200, Laszlo Ersek wrote: >> It turns out that a C-language array containing C-language pointers can >> only be allocated in C: >> >> https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-err... >> >> Rewrite the "arg_string_list" and "free_string_list" functions almost >> entirely in C. >> >> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; >> --- >> generator/golang.ml | 46 ++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 37 insertions(+), 9 deletions(-) >> >> diff --git a/generator/golang.ml b/generator/golang.ml >> index d328abe4ed08..5db478e8a494 100644 >> --- a/generator/golang.ml >> +++ b/generator/golang.ml >> @@ -52,6 +52,39 @@ _go_guestfs_create_flags (unsigned flags) >> { >> return guestfs_create_flags (flags); >> } >> + >> +// >> +// A C-language array of C-language pointers needs to be allocated in C. >> +// https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-err... >> +// >> +typedef char *pChar; >> + >> +pChar *allocStringArray (size_t nmemb) >> +{ >> + pChar *array; >> + >> + array = malloc (sizeof (pChar) * (nmemb + 1)); >> + array[nmemb] = NULL; >> + return array; >> +} >> + >> +void storeString (pChar *array, size_t idx, pChar string) >> +{ >> + array[idx] = string; >> +} >> + >> +void freeStringArray (pChar *array) >> +{ >> + pChar *position; >> + pChar string; >> + >> + position = array; >> + while ((string = *position) != NULL) { >> + free (string); >> + position++; >> + } >> + free (array); >> +} >> */ >> import \"C\" >> >> @@ -148,12 +181,11 @@ func (g *Guestfs) Close () *GuestfsError { >> * C strings and golang []string. >> */ >> func arg_string_list (xs []string) **C.char { >> - r := make ([]*C.char, 1 + len (xs)) >> + r := C.allocStringArray (C.size_t (len (xs))) >> for i, x := range xs { >> - r[i] = C.CString (x) >> + C.storeString (r, C.size_t (i), C.CString (x)); >> } >> - r[len (xs)] = nil >> - return &r[0] >> + return (**C.char) (r) >> } > > This could be done purely in Go without helper functions > > 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 array_size(arr **C.char, len int) C.size_t { > return C.size_t(unsafe.Sizeof(*arr) * (1 + uintptr(len))) > } > > func arg_string_list2(xs []string) **C.char { > var r **C.char > r = (**C.char)(C.malloc(array_size(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 > } I'm a total noob in Go and I find the syntax very difficult to read. In addition to that, I don't have the slightest idea about the Go runtime. (What I do know however is that the #cgo parser is not a full-blown C language parser; for example it does not accept "sizeof variable" without parens, plus see <https://github.com/golang/go/issues/14210> for another example.) The end result is that I wanted to get out of Go-land as quickly and as *simply* as possible (meaning the syntax of calling C code), and then to do the actual business in C. This is the Go binding of a C library, so I feel this approach is justified.

From my POV maintaining Go bindings though, I think that the desirable

>> func count_string_list (argv **C.char) int { >> @@ -167,11 +199,7 @@ func count_string_list (argv **C.char) int { >> } >> >> func free_string_list (argv **C.char) { >> - for *argv != nil { >> - //C.free (*argv) >> - argv = (**C.char) (unsafe.Pointer (uintptr (unsafe.Pointer (argv)) + >> - unsafe.Sizeof (*argv))) >> - } >> + C.freeStringArray ((*C.pChar) (argv)) >> } > > This can be done entirely in Go > > func free_string_list (argv **C.char) { > for i := 0; ; i++ { > str := array_elem(argv, i) > if *str == nil { > break > } > C.free(unsafe.Pointer(*str)) > } > C.free(unsafe.Pointer(argv)) > } I guess it "can", but *should* it? In libguestfs? I think (for any language binding at all, not just Go's), the bindings should be as thin as possible in the other languages, and most of the business should be in C.