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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Bamvor Jian Zhang writes ("[PATCH] [v2] 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.

It's looking pretty good.

> 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)
> +{
...
> +    tty = libxl__xs_read(gc, XBT_NULL, tty_path);
> +    if (tty) 
> +        *path = strdup(tty);
> +    else
> +        rc = ERROR_FAIL;

Firstly, you need to log something on error here or this function will
fail without logging anything if the console is broken.

Secondly, you should use
    libxl__strdup(0, tty)
to get the right error behaviour (if strdup's malloc fails).  Ie,

Thirdly, this is a rather odd error handling pattern.  I would write
       tty = libxl__xs_read(gc, XBT_NULL, tty_path);
       if (!tty) {
           LOGE(ERROR,"unable to read console tty path `%s'",tty_path);
           rc = ERROR_FAIL;
           goto out;
       }
leaving the main flow to carry on:
       *path = libxl__strdup(0, tty);
       rc = 0;

Fourthly, you can then leave rc uninitialised at the top of the
function.  Any path which as a result gets to the exit with it
uninitialised will produce a compiler warning which would previously
have been erroneously suppressed.

> +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm, 
> +                                       uint32_t *domid_out, int 
> *cons_num_out, 
> +                                       libxl_console_type *type_out)
>  {
...
>              break;
>          case -1:
> -            LOG(ERROR,"unable to get domain type for 
> domid=%"PRIu32,domid_vm);
> +            LOG(ERROR,"unable to get domain type for domid=%"PRIu32, 
> domid_vm);

Unrelated whitespace change.

> +int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +{
> +    uint32_t domid_out;
> +    int cons_num_out;
> +    libxl_console_type type_out;

As Ian C says, these shouldn't be called "*_out".

> diff -r ab86fc0e2b45 -r d67ab7c543d5 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h     Tue May 22 11:55:02 2012 +0100
> +++ b/tools/libxl/libxl.h     Wed May 23 14:27:57 2012 +0800
> @@ -601,6 +601,16 @@ int libxl_console_exec(libxl_ctx *ctx, u
>   * case of HVM guests, and before libxl_run_bootloader in case of PV
>   * guests using pygrub. */
>  int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm);
> +/* libxl_console_get_tty retrieves the specified domain's console tty path
> + * and stores it in path. Caller is responsible for freeing the memory.
> + */
> +int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> +                          libxl_console_type type, char **path);
> +/* libxl_primary_console_get_tty retrieves the specified domain's primary 
> + * console tty path and stores it in path. Caller is responsible for freeing
> + * the memory.
> + */
> +int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm, char 
> **path);
>  
>  /* May be called with info_r == NULL to check for domain's existance */
>  int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r,

A formatting nit: I think if we're going to have multi-line comments
associated with functions we should have a blank line either side of
the information about the function to visually associate the comment
with the right function.

Thanks,
Ian.

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