Re: [PATCH v2 3/5] golang/xenlight: Add host-related functionality

[Ronald Rojas <ronladred@gmail.com>]

On Fri, Mar 03, 2017 at 02:54:56PM +0000, George Dunlap wrote:
> On 02/03/17 16:07, 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 <ronladred@gmail.com>
> 
> Looks good -- just two minor comments...
> 
> > ---
> > Changes since last version
> > 
> > - Refactored creating Physinfo and VersionInfo types into
> > seperate toGo() functions.
> > 
> > - Used libxl_<type>_init() and libxl_<type>_dispose() on IDL
> > type physinfo
> > 
> > - Whitespace fixes
> > 
> > CC: xen-devel@lists.xen.org
> > CC: george.dunlap@citrix.com
> > CC: ian.jackson@eu.citrix.com
> > CC: wei.liu2@citrix.com
> > ---
> > ---
> >  tools/golang/xenlight/xenlight.go | 200 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 200 insertions(+)
> > 
> > diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
> > index cbd3527..63cc805 100644
> > --- a/tools/golang/xenlight/xenlight.go
> > +++ b/tools/golang/xenlight/xenlight.go
> > @@ -106,6 +106,103 @@ type Context struct {
> >  	ctx *C.libxl_ctx
> >  }
> >  
> > +type Hwcap []C.uint32_t
> > +
> > +func (chwcap C.libxl_hwcap) CToGo() (ghwcap Hwcap) {
> 
> Was there a reason you left this as CToGo() rather than just toGo()?
> 

No. This should be changed to toGo()

[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
> > +	C.libxl_physinfo_init(&cphys)
> > +
> > +	ret := C.libxl_get_physinfo(Ctx.ctx, &cphys)
> > +
> > +	if ret < 0 {
> > +		err = Error(ret)
> > +		return
> > +	}
> > +	physinfo = cphys.toGo()
> > +	C.libxl_physinfo_dispose(&cphys)
> 
> I think it would probably be more idiomatic to write "defer
> C.libxl_physinfo_dispose()" just after the physinfo_init.
> 
> If the init() actually allocated any memory, the current code would
> return without disposing of it if there was an error.  `defer` avoids
> that by ensuring that *all* return paths call the clean-up function.

Yep. Using defer is the better approach here. I will change
it in all instances where I use the init/dispose functions.

> 
> Other than that, looks great, thanks!
> 
>  -George
> 

Thanks,
Ronald

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel