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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-06-01 at 08:21 +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 v2:
>  * using ERROR_INVAL instead of ERROR_FAIL in libxl_console_get_tty and
>  libxl__primary_console_find if need.
>  * remove _out from some value name in libxl_primary_console_exec
>  * add error handler and log message in libxl_console_get_tty.
>  BUT, NOT update strdup to libxl__strdup. bacause libxl__strdup(0, tty) will 
> lead to null pointer access in CTX maco.
>  * add empty line between my comment and other function.
> 
> diff -r d7318231cfe3 -r 3496f86000b8 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c     Thu May 31 10:18:52 2012 +0200
> +++ b/tools/libxl/libxl.c     Fri Jun 01 15:10:45 2012 +0800
> @@ -1188,7 +1188,8 @@ out:
>      return rc;
>  }
>  
> -int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, 
> libxl_console_type type)
> +int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
> +                       libxl_console_type type)
>  {
>      GC_INIT(ctx);
>      char *p = libxl__sprintf(gc, "%s/xenconsole", 
> libxl__private_bindir_path());
> @@ -1214,25 +1215,82 @@ out:
>      return ERROR_FAIL;
>  }
>  
> -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)
> +{
> +    GC_INIT(ctx);
> +    char *dom_path = 0;
> +    char *tty_path = 0;
> +    char *tty = 0;

Is the compiler giving false positives about using these without
initialising them?

Otherwise these initialisations are simply hiding what would a useful
warning from the compiler.

Or is there some deliberate path by which these can be used without
first being set to something explicit?

> +    int rc;
> +
> +    dom_path = libxl__xs_get_dompath(gc, domid);
> +    if (!dom_path) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    switch (type) {
> +    case LIBXL_CONSOLE_TYPE_SERIAL:
> +        tty_path = GCSPRINTF("%s/serial/0/tty", dom_path);
> +        break;
> +    case LIBXL_CONSOLE_TYPE_PV:
> +        if (cons_num == 0)
> +            tty_path = GCSPRINTF("%s/console/tty", dom_path);
> +        else
> +            tty_path = GCSPRINTF("%s/device/console/%d/tty", dom_path, 
> +                                cons_num);
> +        break;
> +    default:
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    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;
> +    }
> +
> +    *path = strdup(tty);
> +    if (!*path)
> +        libxl__alloc_failed(CTX, __func__, strlen(*path), 1);
> +
> +    rc = 0;
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +static int libxl__primary_console_find(libxl_ctx *ctx, uint32_t domid_vm, 
> +                                       uint32_t *domid, int *cons_num, 
> +                                       libxl_console_type *type)
>  {
>      GC_INIT(ctx);
>      uint32_t stubdomid = libxl_get_stubdom_id(ctx, domid_vm);
> -    int rc;
> -    if (stubdomid)
> -        rc = libxl_console_exec(ctx, stubdomid,
> -                                STUBDOM_CONSOLE_SERIAL, 
> LIBXL_CONSOLE_TYPE_PV);
> -    else {
> +    int rc = 0;

This initialisation also potentially hides errors, I think. Better to
explicitly set in the cases where we succeed.

> +
> +    if (stubdomid) {
> +        *domid = stubdomid;
> +        *cons_num = STUBDOM_CONSOLE_SERIAL;
> +        *type = LIBXL_CONSOLE_TYPE_PV;
> +    } else {
>          switch (libxl__domain_type(gc, domid_vm)) {
>          case LIBXL_DOMAIN_TYPE_HVM:
> -            rc = libxl_console_exec(ctx, domid_vm, 0, 
> LIBXL_CONSOLE_TYPE_SERIAL);
> +            *domid = domid_vm;
> +            *cons_num = 0;
> +            *type = LIBXL_CONSOLE_TYPE_SERIAL;
>              break;
>          case LIBXL_DOMAIN_TYPE_PV:
> -            rc = libxl_console_exec(ctx, domid_vm, 0, LIBXL_CONSOLE_TYPE_PV);
> +            *domid = domid_vm;
> +            *cons_num = 0;
> +            *type = LIBXL_CONSOLE_TYPE_PV;
>              break;
>          case -1:
> -            LOG(ERROR,"unable to get domain type for 
> domid=%"PRIu32,domid_vm);
> -            rc = ERROR_FAIL;
> +            LOG(ERROR,"unable to get domain type for domid=%"PRIu32, 
> domid_vm);
> +            rc = ERROR_INVAL;
>              break;
>          default:
>              abort();
> @@ -1242,6 +1300,31 @@ int libxl_primary_console_exec(libxl_ctx
>      return rc;
>  }
>  


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