golang/xenlight: Fix type issues with recent Go version

Commit Message

Nicolas Belouin June 27, 2019, 7:58 a.m. UTC

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.
---
 tools/golang/xenlight/xenlight.go | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

George Dunlap June 27, 2019, 4:24 p.m. UTC | #1

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.

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

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

The other two changes look good, thanks.

 -George

Nicolas Belouin June 28, 2019, 7:22 a.m. UTC | #2

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

Patch

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)))
 	return
 }
 
@@ -1170,7 +1170,7 @@  func (Ctx *Context) ConsoleGetTty(id Domid, consNum int, conType ConsoleType) (p
 		err = Error(-ret)
 		return
 	}
-	defer C.free(cpath)
+	defer C.free(unsafe.Pointer(cpath))
 
 	path = C.GoString(cpath)
 	return
@@ -1190,7 +1190,7 @@  func (Ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error)
 		err = Error(-ret)
 		return
 	}
-	defer C.free(cpath)
+	defer C.free(unsafe.Pointer(cpath))
 
 	path = C.GoString(cpath)
 	return