Re: [Xen-devel] [PATCH] golang/xenlight: Fix type issues with recent Go version

[Nicolas Belouin <nicolas.belouin@xxxxxxxxx>]

On 27/06 17:24, George Dunlap wrote:
> On 6/27/19 8:58 AM, Nicolas Belouin wrote:
> > Go is doing more type check (even when using CGo), so these incorrect
> > use of `unsafe.Pointer` as well as the lack of `unsafe.Pointer` for
> > these calls no longer compile with recent Go versions.
> > 
> > This does *not* break compatibility with older Go version.
> Need a SoB here.

Indeed I missed that one.

> 
> Also, I think a slightly more descriptive commit message would be
> helpful; something like:
> 
> ---
> Newer versions of Go have become stricter on acceptable pointer
> conversions.  Specifically, the following two conversions are no longer
> allowed:
> 
> - unsafe.Pointer being automatically cast to another type
> - A pointer type other than unsafe.Pointer being automatically cast to C
> void *
> 
> Fix this by adding explicit casts where necessary.
> ---

This is indeed more understandable, in fact Golang does not accept any
implicit conversion and these working were more likely a bug in CGo. I
will take your suggested commit message as a base for a V2

> 
> > ---
> >  tools/golang/xenlight/xenlight.go | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/golang/xenlight/xenlight.go 
> > b/tools/golang/xenlight/xenlight.go
> > index 53534d047e..e281328d43 100644
> > --- a/tools/golang/xenlight/xenlight.go
> > +++ b/tools/golang/xenlight/xenlight.go
> > @@ -854,7 +854,7 @@ func (Ctx *Context) Open() (err error) {
> >     }
> >  
> >     ret := C.libxl_ctx_alloc(&Ctx.ctx, C.LIBXL_VERSION,
> > -           0, unsafe.Pointer(Ctx.logger))
> > +           0, (*C.struct_xentoollog_logger)(unsafe.Pointer(Ctx.logger)))
> >  
> >     if ret != 0 {
> >             err = Error(-ret)
> > @@ -869,7 +869,7 @@ func (Ctx *Context) Close() (err error) {
> >     if ret != 0 {
> >             err = Error(-ret)
> >     }
> > -   C.xtl_logger_destroy(unsafe.Pointer(Ctx.logger))
> > +   
> > C.xtl_logger_destroy((*C.struct_xentoollog_logger)(unsafe.Pointer(Ctx.logger)))
> 
> I'm wondering if a better approach here would be to have Ctx.logger be
> type C.xentoollog_logger, and just do the cast from
> xentoollog_logger_stdiostream once when creating the logger.

This looks like a better approach as Ctx should not expect a specific
xentoollog_logger type (even though there is only one for now)

> 
> The other two changes look good, thanks.
> 
>  -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel