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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Bamvor Jian Zhang writes ("[PATCH] [PATCH] libxl: Add API to retrieve domain 
console tty"):
> This api retrieve domain console from xenstore. With this new api, it is easy 
> to implement "virsh console" command in libvirt libxl driver.

Thanks.  This is coming along in the right direction.  I have just a
few comments.

> Signed-off-by: Bamvor Jian Zhang <bjzhang@xxxxxxxx>
> 
> diff -r f8279258e3c9 -r a1dc373628e4 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c     Mon May 14 17:15:36 2012 +0100
> +++ b/tools/libxl/libxl.c     Mon May 21 18:51:17 2012 +0800
> @@ -1552,6 +1552,61 @@ out:
>      return rc;
>  }
>  
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> +                          libxl_console_type type, char **path)
> +{
...
> +            tty_path = libxl__sprintf(gc, "%s/console/tty", dom_path);

You could profitably make use of the GCSPRINTF helper macro here.

> +        else
> +            tty_path = libxl__sprintf(gc, "%s/device/console/%d/tty", 
> dom_path, cons_num);

This line needs to be reformatted to be less than 75-80 characters.
There are a couple of other long lines in your patch too.

> +
> +    tty = libxl__xs_read(gc, XBT_NULL, tty_path);
> +    *path = strdup(tty);
> +

You need to check for errors here.  libxl__xs_read is not infallible.

> +int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char 
> **path)
> +{
> +    GC_INIT(ctx);
> +    uint32_t stubdomid = libxl_get_stubdom_id(ctx, domid_vm);
> +    int rc;
> +    if (stubdomid)
> +        rc = libxl_console_get_tty(ctx, stubdomid,
> +                STUBDOM_CONSOLE_SERIAL, LIBXL_CONSOLE_TYPE_PV, path);
> +    else {
> +        switch (libxl__domain_type(gc, domid_vm)) {
> +        case LIBXL_DOMAIN_TYPE_HVM:
> +            rc = libxl_console_get_tty(ctx, domid_vm, 0, 
> LIBXL_CONSOLE_TYPE_SERIAL, path);

This recapitules the logic in libxl_primary_console_exec.  I think you
should refactor this so that the substance of it is in a single
function eg

  int libxl__primary_console_find(libxl__gc *gc, uint32_t domid_vm
                                  int *num_out, libxl_console_type *type_out);

Also aborting if libxl__domain_type fails is not correct, but if you
refactor this and use the existing code in the middle of
libxl_primary_console_exec you'll handle that case correctly.

Thanks,
Ian.

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