Re: [Xen-devel] [PATCH v4 3/3] golang/xenlight: Implement DomainCreateNew

[George Dunlap <george.dunlap@citrix.com>]

On 3/11/20 2:59 PM, Nick Rosbrook wrote:
> Looks good, I just have two small comments:
> 
>> diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go
>> index 56fa31fd7b..808b4a327c 100644
>> --- a/tools/golang/xenlight/xenlight.go
>> +++ b/tools/golang/xenlight/xenlight.go
>> @@ -1111,3 +1111,24 @@ func (Ctx *Context) PrimaryConsoleGetTty(domid uint32) (path string, err error)
>>         path = C.GoString(cpath)
>>         return
>>  }
>> +
>> +// int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
>> +//                             uint32_t *domid,
>> +//                             const libxl_asyncop_how *ao_how,
>> +//                             const libxl_asyncprogress_how *aop_console_how)
> 
> Conventionally, we want to have comments for exported functions along
> the lines of:
> 
>     // DomainCreateNew creates a new domain with config, and returns
> its Domid on success.
>     // A non-nil error is returned if config cannot be marshaled, or
> an error occurs within libxl.
> 
> Besides being easier to read, it makes documentation more clear on
> godoc/pkg.go.dev.

Yes, absolutely, that's something we need to change before we declare
"1.0".  But that should probably be done in a series which changes all
such comments together.

>> +func (Ctx *Context) DomainCreateNew(config *DomainConfig) (Domid, error) {
> 
> Capitalizing "Ctx" here is a little weird to me. Since it's only the
> receiver name, there's no effect, but since capitalized identifiers
> have special-meaning in other contexts, I would avoid doing this.

I c&p'd this from another method and just changed the signature.  It
probably would be good to make all of them lower-case.  I may change
this one on check in though.

> I only point those out in case you want to change it on check-in. Besides that,
> 
> Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Thanks!

 -George

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