[v2] golang/xenlight: Fix type issues with recent Go version
diff mbox series

Message ID 20190628083148.1747-1-nicolas.belouin@gandi.net
State New, archived
Headers show
Series
  • [v2] golang/xenlight: Fix type issues with recent Go version
Related show

Commit Message

Nicolas Belouin June 28, 2019, 8:31 a.m. UTC
Newer versions of Go have become stricter on enforcing the no implicit
conversions policy when using CGo.
Specifically, the following two conversions are no longer allowed:

- unsafe.Pointer being automatically cast to any C pointer
- A pointer type other than unsafe.Pointer being automatically cast to C
void *

Fix this by adding explicit casts where necessary.

Signed-off-by: Nicolas Belouin <nicolas.belouin@gandi.net>
---
 tools/golang/xenlight/xenlight.go | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

George Dunlap June 28, 2019, 5:05 p.m. UTC | #1
On 6/28/19 9:31 AM, Nicolas Belouin wrote:
> Newer versions of Go have become stricter on enforcing the no implicit
> conversions policy when using CGo.
> Specifically, the following two conversions are no longer allowed:
> 
> - unsafe.Pointer being automatically cast to any C pointer
> - A pointer type other than unsafe.Pointer being automatically cast to C
> void *
> 
> Fix this by adding explicit casts where necessary.

This looks good, except now the commit message is wrong: We're no longer
simply adding casts; we're changing Context.logger from
*C.xentoollog_logger_stdiostream to *C.xentoollog_logger.  That needs to
be mentioned in the commit message.

I think given what you said about automatic pointer conversion
theoretically never being OK, I might say this instead:

---
Theoretically Go has never allowed automatic pointer conversions; but in
practice earlier versions let some conversions slide.  Newer compilers
are more strict, so make sure that all pointers are converted explicitly
the appropriate types.

Change Context.logger's type to *C.xentoollog_logger, as that's the more
generic type, and results in fewer manual pointer conversions.
---

If you're OK with the above I can change it on check-in.

Otherwise:

Reviewed-by:  George Dunlap <george.dunlap@citrix.com>
Nicolas Belouin July 1, 2019, 6:21 a.m. UTC | #2
On 28/06 18:05, George Dunlap wrote:
> On 6/28/19 9:31 AM, Nicolas Belouin wrote:
> > Newer versions of Go have become stricter on enforcing the no implicit
> > conversions policy when using CGo.
> > Specifically, the following two conversions are no longer allowed:
> > 
> > - unsafe.Pointer being automatically cast to any C pointer
> > - A pointer type other than unsafe.Pointer being automatically cast to C
> > void *
> > 
> > Fix this by adding explicit casts where necessary.
> 
> This looks good, except now the commit message is wrong: We're no longer
> simply adding casts; we're changing Context.logger from
> *C.xentoollog_logger_stdiostream to *C.xentoollog_logger.  That needs to
> be mentioned in the commit message.
> 
> I think given what you said about automatic pointer conversion
> theoretically never being OK, I might say this instead:
> 
> ---
> Theoretically Go has never allowed automatic pointer conversions; but in
> practice earlier versions let some conversions slide.  Newer compilers
> are more strict, so make sure that all pointers are converted explicitly
> the appropriate types.
> 
> Change Context.logger's type to *C.xentoollog_logger, as that's the more
> generic type, and results in fewer manual pointer conversions.
> ---
> 
> If you're OK with the above I can change it on check-in.

I'm OK with the change, thanks.

> 
> Otherwise:
> 
> Reviewed-by:  George Dunlap <george.dunlap@citrix.com>

Patch
diff mbox series

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index 53534d047e..a2af6f6ef9 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -122,7 +122,7 @@  type Uuid C.libxl_uuid
 
 type Context struct {
 	ctx    *C.libxl_ctx
-	logger *C.xentoollog_logger_stdiostream
+	logger *C.xentoollog_logger
 }
 
 type Hwcap []C.uint32_t
@@ -847,14 +847,15 @@  func (Ctx *Context) Open() (err error) {
 		return
 	}
 
-	Ctx.logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)
+	Ctx.logger = (*C.xentoollog_logger)(unsafe.Pointer(
+		C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)))
 	if Ctx.logger == nil {
 		err = fmt.Errorf("Cannot open stdiostream")
 		return
 	}
 
 	ret := C.libxl_ctx_alloc(&Ctx.ctx, C.LIBXL_VERSION,
-		0, unsafe.Pointer(Ctx.logger))
+		0, Ctx.logger)
 
 	if ret != 0 {
 		err = Error(-ret)
@@ -869,7 +870,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(Ctx.logger)
 	return
 }
 
@@ -1170,7 +1171,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 +1191,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