Re: [Xen-devel] [PATCH] [v2] libxl: Add API to retrieve domain console tty

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [PATCH] [v2] libxl: Add API to retrieve domain 
console tty"):
> On Wed, 2012-05-23 at 07:35 +0100, Bamvor Jian Zhang wrote:
> > This api retrieve domain console from xenstore. With this new api, it is 
> > easy to implement "virsh console" command in libvirt libxl driver.
> > 
> > Signed-off-by: Bamvor Jian Zhang <bjzhang@xxxxxxxx>
> > 
> > Changes since v1:
> >  * Replace function libxl__sprintf with macro GSPRINTF
> >  * check return value and handle error for libxl__xs_read and 
> > libxl__domain_type
> >  * merge common code for libxl_primary_console_get_tty and
> >    libxl_primary_console_exec as libxl__primary_console_find
> >  * Reformat code to be less than 80 characters.
> > 
> > diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c   Tue May 22 11:55:02 2012 +0100
> > +++ b/tools/libxl/libxl.c   Wed May 23 14:27:57 2012 +0800
> > @@ -1188,7 +1188,8 @@ out:
...
> > -int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> > +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> > +                          libxl_console_type type, char **path)
> > +{
...
> > +    switch (type) {
> > +    case LIBXL_CONSOLE_TYPE_SERIAL:
...
> > +    default:
> > +        rc = ERROR_FAIL;
> 
> Strictly this ought to be ERROR_INVAL.

Yes.

> > +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm, 
> 
> Generally since this is an internal function it should take a libxl__gc
> *gc not a ctx and drop the GC_INIT and GC_FREE. You can use the CTX
> macro to get a ctx where you need one.

I think this is fine.  In fact in the future we might want to make
this a public function (but not now).

> However since the two callers are thinish wrappers around this and you'd
> then need the GC_INIT, GC_FREE stuff there I'm inclined to suggest we
> make an exception here and leave it as is, Ian J what do you think?

I think there is no rule against internal functions taking ctx.  gc is
more conventional but here I think the balance of convenience is in
favour of what Bamvor has done.

Particularly since the outer two functions are so simple.

> There isn't much here which warrants a resend IMHO, if Ian J is happy
> with the libxl__primary_console_find interface (as discussed above) I'd
> be inclined to take it as is unless you really want to do a respin.

I think my comment about the log message ought to be addressed with a
resend.

The nits could have been fixed in-tree at our leisure but if we're
going to have a resend it would be best to address them all.

Thanks,
Ian.

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