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

[Daniel P. Berrangé]

On Mon, Sep 20, 2021 at 05:11:38PM +0200, Laszlo Ersek wrote:
> 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-error-cgo-argument-has-go-pointer-to-go-pointer
> >>
> >> Rewrite the "arg_string_list" and "free_string_list" functions almost
> >> entirely in C.
> >>
> >> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> >> ---
> >>  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-error-cgo-argument-has-go-pointer-to-go-pointer
> >> +//
> >> +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.

I can understand that POV if you're most comfortable being C developer.
>From my POV maintaining Go bindings though, I think that the desirable
to use Go code to the greatest extent possible. I'd only use C wrapper
functions if it was the only solution - eg where you need 2 consequetive
C API calls in the same thread for TLS, or where you need to deal with
things like variadic functions.


> >>  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.

To me the great benefit of Go's native API layer compared to languages like
Python/Perl/Ruby is that you can do nearly everything purely in Go code and
so almost never need to create C wrapper functions.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|