Re: [Xen-devel] [PATCH RFC 3/8] golang/xenlight: Add host-related functionality
On 18/01/17 19:56, Ronald Rojas wrote:
> Add calls for the following host-related functionality:
> - libxl_get_max_cpus
> - libxl_get_online_cpus
> - libxl_get_max_nodes
> - libxl_get_free_memory
> - libxl_get_physinfo
> - libxl_get_version_info
>
> Include Golang versions of the following structs:
> - libxl_physinfo as Physinfo
> - libxl_version_info as VersionInfo
> - libxl_hwcap as Hwcap
>
> Signed-off-by: Ronald Rojas
This looks good, and I think could be checked in once rebased on
previous changes. One comment for future direction...
> ---
> tools/golang/xenlight/xenlight.go | 185
> ++
> 1 file changed, 185 insertions(+)
>
> diff --git a/tools/golang/xenlight/xenlight.go
> b/tools/golang/xenlight/xenlight.go
> index d58f8b8..6b04850 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -109,6 +109,62 @@ type Context struct {
> ctx *C.libxl_ctx
> }
>
> +type Hwcap []C.uint32_t
> +
> +func hwcapCToGo(chwcap C.libxl_hwcap) (ghwcap Hwcap) {
> + // Alloc a Go slice for the bytes
> + size := 8
> + ghwcap = make([]C.uint32_t, size)
> +
> + // Make a slice pointing to the C array
> + mapslice := (*[1 <<
> 30]C.uint32_t)(unsafe.Pointer(&chwcap[0]))[:size:size]
> +
> + // And copy the C array into the Go array
> + copy(ghwcap, mapslice)
> +
> + return
> +}
In my own copy of the library, I was experimenting with using methods to
do this, rather than making standalone functions that embed the name in
the type. Something like this:
func (c C.libxl_dominfo) toGo() (g Dominfo) {
g.Uuid = Uuid(c.uuid)
[etc]
}
That way you can do cdi.toGo() rather than dominfoCToGo(cdi). The
syntax looks a lot cleaner and more consistent.
What do you think?
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 3/8] golang/xenlight: Add host-related functionality
On 23/01/17 16:43, Ronald Rojas wrote: > Add calls for the following host-related functionality: > - libxl_get_max_cpus > - libxl_get_online_cpus > - libxl_get_max_nodes > - libxl_get_free_memory > - libxl_get_physinfo > - libxl_get_version_info > > Include Golang versions of the following structs: > - libxl_physinfo as Physinfo > - libxl_version_info as VersionInfo > - libxl_hwcap as Hwcap > > Signed-off-by: Ronald Rojas Oh, last thing: It's probably worth running "go fmt xenlight.go" for each patch. That will catch little things like lack of spaces between things, and aligning the structures nicely. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 3/8] golang/xenlight: Add host-related functionality
On 23/01/17 16:43, Ronald Rojas wrote:
> Add calls for the following host-related functionality:
> - libxl_get_max_cpus
> - libxl_get_online_cpus
> - libxl_get_max_nodes
> - libxl_get_free_memory
> - libxl_get_physinfo
> - libxl_get_version_info
>
> Include Golang versions of the following structs:
> - libxl_physinfo as Physinfo
> - libxl_version_info as VersionInfo
> - libxl_hwcap as Hwcap
>
> Signed-off-by: Ronald Rojas
Mostly looks good! One final comment:
[snip]
> +//int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
> +func (Ctx *Context) GetPhysinfo() (physinfo *Physinfo, err error) {
> + err = Ctx.CheckOpen()
> + if err != nil {
> + return
> + }
> + var cphys C.libxl_physinfo
> +
> + ret := C.libxl_get_physinfo(Ctx.ctx, &cphys)
So I actually forgot that you're actually supposed to always call
"libxl__init()" on IDL-generated types before using them, and
"libxl__dispose()" when you're done with them. As it happens in
this case, the struct contains no pointers to sub-structs; but if that
changes in the future, this would leak the memory.
Everything else is good, thanks.
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 3/8] golang/xenlight: Add host-related functionality
On 23/01/17 16:43, Ronald Rojas wrote:
> Add calls for the following host-related functionality:
> - libxl_get_max_cpus
> - libxl_get_online_cpus
> - libxl_get_max_nodes
> - libxl_get_free_memory
> - libxl_get_physinfo
> - libxl_get_version_info
>
> Include Golang versions of the following structs:
> - libxl_physinfo as Physinfo
> - libxl_version_info as VersionInfo
> - libxl_hwcap as Hwcap
>
> Signed-off-by: Ronald Rojas
> ---
> tools/golang/xenlight/xenlight.go | 186
> ++
> 1 file changed, 186 insertions(+)
>
> diff --git a/tools/golang/xenlight/xenlight.go
> b/tools/golang/xenlight/xenlight.go
> index eee2254..92410a8 100644
> --- a/tools/golang/xenlight/xenlight.go
> +++ b/tools/golang/xenlight/xenlight.go
> @@ -108,6 +108,63 @@ type Context struct {
> ctx *C.libxl_ctx
> }
>
> +type Hwcap []C.uint32_t
> +
> +func (chwcap C.libxl_hwcap)CToGo() (ghwcap Hwcap) {
Sorry... I'm coming back to this after a bit of time and remembering a
bunch of things as I'm going along.
I don't think we want this method to be available publicly, so this
should probably be 'cToGo' (Or perhaps just 'toGo', since it's already a
C structure.)
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
