diff mbox series

[06/24] golang/xenlight: re-name Bitmap marshaling functions

Message ID e6f9c93bd09a247fb0675b01aae8c1f2819f9a70.1570456846.git.rosbrookn@ainfosec.com (mailing list archive)
State Superseded
Headers show
Series generated Go libxl bindings using IDL | expand

Commit Message

Nick Rosbrook Oct. 7, 2019, 3:12 p.m. UTC 
From: Nick Rosbrook <rosbrookn@ainfosec.com>

Re-name and modify signature of toGo function to fromC. The reason for
using 'fromC' rather than 'toGo' is that it is not a good idea to define
methods on the C types. Also, add error return type to Bitmap's toC function.

Finally, as code-cleanup, re-organize the Bitmap type's comments as per
Go conventions.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>

 tools/golang/xenlight/xenlight.go | 93 ++++++++++++++++---------------
 1 file changed, 48 insertions(+), 45 deletions(-)

Comments

George Dunlap Nov. 13, 2019, 3:21 p.m. UTC | #1 
On 10/7/19 4:12 PM, Nick Rosbrook wrote:
> From: Nick Rosbrook <rosbrookn@ainfosec.com>
> 
> Re-name and modify signature of toGo function to fromC. The reason for
> using 'fromC' rather than 'toGo' is that it is not a good idea to define
> methods on the C types. Also, add error return type to Bitmap's toC function.
> 
> Finally, as code-cleanup, re-organize the Bitmap type's comments as per
> Go conventions.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

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

With one minor comment...


> +func (bm *Bitmap) fromC(cbm *C.libxl_bitmap) error {
> +	// Alloc a Go slice for the bytes
> +	size := int(cbm.size)
> +	bm.bitmap = make([]C.uint8_t, size)
> +
> +	// Make a slice pointing to the C array
> +	mapslice := (*[1 << 30]C.uint8_t)(unsafe.Pointer(cbm._map))[:size:size]
> +
> +	// And copy the C array into the Go array
> +	copy(bm.bitmap, mapslice)
> +
> +	return nil
> +}
> +
> +func (bm *Bitmap) toC() (C.libxl_bitmap, error) {
> +	var cbm C.libxl_bitmap
> +
> +	size := len(bm.bitmap)
> +	cbm.size = C.uint32_t(size)
> +	cbm._map = (*C.uint8_t)(C.malloc(C.ulong(cbm.size) * C.sizeof_uint8_t))
> +	cslice := (*[1 << 31]C.uint8_t)(unsafe.Pointer(cbm._map))[:size:size]

Any particular reason to use `cslice` here rather than `mapslice` (or
vice versa)?

Not a big deal, but since they're of the came element in the C struct,
it seems like it would be better to give them the same name.  (Don't
have a strong opinion on which one).

 -George
Nick Rosbrook Nov. 13, 2019, 8:53 p.m. UTC | #2 
> Any particular reason to use `cslice` here rather than `mapslice` (or
> vice versa)?
>
> Not a big deal, but since they're of the came element in the C struct,
> it seems like it would be better to give them the same name.  (Don't
> have a strong opinion on which one).

IIRC, I found the name `mapslice` a little confusing, since it wasn't
of type []map[T1]T2. But, as to the inconsistent naming between the
two functions, I agree. I'll name them both `cslice`.

-NR
George Dunlap Nov. 14, 2019, 11:44 a.m. UTC | #3 
On 11/13/19 8:53 PM, Nick Rosbrook wrote:
>> Any particular reason to use `cslice` here rather than `mapslice` (or
>> vice versa)?
>>
>> Not a big deal, but since they're of the came element in the C struct,
>> it seems like it would be better to give them the same name.  (Don't
>> have a strong opinion on which one).
> 
> IIRC, I found the name `mapslice` a little confusing, since it wasn't
> of type []map[T1]T2. But, as to the inconsistent naming between the
> two functions, I agree. I'll name them both `cslice`.

I think I named it that way because it's a slice fake-up of the struct
element `map`.  But it certainly has the risk of confusing people, and
I'm a big believer in absolutely minimizing useless cognitive load. :-)

Another approach might be to just choose a short "metavariable" for this
sort of thing; i.e., if you use `i` and `j`, everyone knows they're
going to be indexes for nested loops.  `cslice` is OK but is probably
longer than necessary.

Anyway, I'll let you paint this bike shed whatever color you want; just
make sure all sheds for the same struct are the same color. ;-)

 -George
diff mbox series

Patch

diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
index 8196a42855..09fcdca5d1 100644
--- a/tools/golang/xenlight/xenlight.go
+++ b/tools/golang/xenlight/xenlight.go
@@ -234,19 +234,48 @@  func (kvl KeyValueList) toC() (C.libxl_key_value_list, error) {
 	return ckvl, nil
 }
 
-// typedef struct {
-//     uint32_t size;          /* number of bytes in map */
-//     uint8_t *map;
-// } libxl_bitmap;
+// Bitmap represents a libxl_bitmap.
+//
 // Implement the Go bitmap type such that the underlying data can
 // easily be copied in and out.  NB that we still have to do copies
 // both directions, because cgo runtime restrictions forbid passing to
 // a C function a pointer to a Go-allocated structure which contains a
 // pointer.
 type Bitmap struct {
+	// typedef struct {
+	//     uint32_t size;          /* number of bytes in map */
+	//     uint8_t *map;
+	// } libxl_bitmap;
 	bitmap []C.uint8_t
 }
 
+func (bm *Bitmap) fromC(cbm *C.libxl_bitmap) error {
+	// Alloc a Go slice for the bytes
+	size := int(cbm.size)
+	bm.bitmap = make([]C.uint8_t, size)
+
+	// Make a slice pointing to the C array
+	mapslice := (*[1 << 30]C.uint8_t)(unsafe.Pointer(cbm._map))[:size:size]
+
+	// And copy the C array into the Go array
+	copy(bm.bitmap, mapslice)
+
+	return nil
+}
+
+func (bm *Bitmap) toC() (C.libxl_bitmap, error) {
+	var cbm C.libxl_bitmap
+
+	size := len(bm.bitmap)
+	cbm.size = C.uint32_t(size)
+	cbm._map = (*C.uint8_t)(C.malloc(C.ulong(cbm.size) * C.sizeof_uint8_t))
+	cslice := (*[1 << 31]C.uint8_t)(unsafe.Pointer(cbm._map))[:size:size]
+
+	copy(cslice, bm.bitmap)
+
+	return cbm, nil
+}
+
 /*
  * Types: IDL
  *
@@ -447,7 +476,7 @@  func (cci C.libxl_cpupoolinfo) toGo() (gci CpupoolInfo) {
 	gci.PoolName = C.GoString(cci.pool_name)
 	gci.Scheduler = Scheduler(cci.sched)
 	gci.DomainCount = int(cci.n_dom)
-	gci.Cpumap = cci.cpumap.toGo()
+	gci.Cpumap.fromC(&cci.cpumap)
 
 	return
 }
@@ -521,7 +550,10 @@  func (Ctx *Context) CpupoolCreate(Name string, Scheduler Scheduler, Cpumap Bitma
 	var uuid C.libxl_uuid
 	C.libxl_uuid_generate(&uuid)
 
-	cbm := Cpumap.toC()
+	cbm, err := Cpumap.toC()
+	if err != nil {
+		return
+	}
 	defer C.libxl_bitmap_dispose(&cbm)
 
 	ret := C.libxl_cpupool_create(Ctx.ctx, name, C.libxl_scheduler(Scheduler),
@@ -576,7 +608,10 @@  func (Ctx *Context) CpupoolCpuaddCpumap(Poolid uint32, Cpumap Bitmap) (err error
 		return
 	}
 
-	cbm := Cpumap.toC()
+	cbm, err := Cpumap.toC()
+	if err != nil {
+		return
+	}
 	defer C.libxl_bitmap_dispose(&cbm)
 
 	ret := C.libxl_cpupool_cpuadd_cpumap(Ctx.ctx, C.uint32_t(Poolid), &cbm)
@@ -612,7 +647,10 @@  func (Ctx *Context) CpupoolCpuremoveCpumap(Poolid uint32, Cpumap Bitmap) (err er
 		return
 	}
 
-	cbm := Cpumap.toC()
+	cbm, err := Cpumap.toC()
+	if err != nil {
+		return
+	}
 	defer C.libxl_bitmap_dispose(&cbm)
 
 	ret := C.libxl_cpupool_cpuremove_cpumap(Ctx.ctx, C.uint32_t(Poolid), &cbm)
@@ -735,41 +773,6 @@  func (Ctx *Context) CpupoolMakeFree(Cpumap Bitmap) (err error) {
  * Bitmap operations
  */
 
-// Return a Go bitmap which is a copy of the referred C bitmap.
-func (cbm C.libxl_bitmap) toGo() (gbm Bitmap) {
-	// Alloc a Go slice for the bytes
-	size := int(cbm.size)
-	gbm.bitmap = make([]C.uint8_t, size)
-
-	// Make a slice pointing to the C array
-	mapslice := (*[1 << 30]C.uint8_t)(unsafe.Pointer(cbm._map))[:size:size]
-
-	// And copy the C array into the Go array
-	copy(gbm.bitmap, mapslice)
-
-	return
-}
-
-// Must be C.libxl_bitmap_dispose'd of afterwards
-func (gbm Bitmap) toC() (cbm C.libxl_bitmap) {
-	C.libxl_bitmap_init(&cbm)
-
-	size := len(gbm.bitmap)
-	cbm._map = (*C.uint8_t)(C.malloc(C.size_t(size)))
-	cbm.size = C.uint32_t(size)
-	if cbm._map == nil {
-		panic("C.calloc failed!")
-	}
-
-	// Make a slice pointing to the C array
-	mapslice := (*[1 << 30]C.uint8_t)(unsafe.Pointer(cbm._map))[:size:size]
-
-	// And copy the Go array into the C array
-	copy(mapslice, gbm.bitmap)
-
-	return
-}
-
 func (bm *Bitmap) Test(bit int) bool {
 	ubit := uint(bit)
 	if bit > bm.Max() || bm.bitmap == nil {
@@ -1158,8 +1161,8 @@  func (cvci C.libxl_vcpuinfo) toGo() (gvci Vcpuinfo) {
 	gvci.Blocked = bool(cvci.blocked)
 	gvci.Running = bool(cvci.running)
 	gvci.VCpuTime = time.Duration(cvci.vcpu_time)
-	gvci.Cpumap = cvci.cpumap.toGo()
-	gvci.CpumapSoft = cvci.cpumap_soft.toGo()
+	gvci.Cpumap.fromC(&cvci.cpumap)
+	gvci.CpumapSoft.fromC(&cvci.cpumap_soft)
 
 	return
 }