Re: [PATCH v6 for-4.5 1/5] libxl: add support for 'channels'

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Sep 24, 2014 at 09:48:01PM +0100, David Scott wrote:
> A 'channel':
>   - is a low-bandwidth private communication channel that resembles
>     a physical serial port.
>   - is implemented as a PV console with a well-known string name
>     which is used to hook the channel to the appropriate software
>     in the guest (i.e. some kind of guest agent).
>   - has a backend 'connection' which describes what should happen
>     to the data.
> 
> The following 'connection' types are defined:
> 
>  * PTY: the I/O surfaces as a pty in the backend domain
>  * SOCKET: a listening Unix domain socket accepts a connection in
>    the backend domain and proxies
> 
> Channels may be listed but don't currently support hotplug/unplug.
> 
> Signed-off-by: David Scott <dave.scott@citrix.com>
> ---
>  docs/misc/channel.txt                |   95 ++++++++++++
>  docs/misc/console.txt                |   69 ++++++---
>  tools/libxl/libxl.c                  |  268 +++++++++++++++++++++++++++++++---
>  tools/libxl/libxl.h                  |   20 +++
>  tools/libxl/libxl_create.c           |   38 +++--
>  tools/libxl/libxl_dm.c               |   46 +++++-
>  tools/libxl/libxl_internal.h         |   10 +-
>  tools/libxl/libxl_types.idl          |   37 +++++
>  tools/libxl/libxl_types_internal.idl |    5 +
>  9 files changed, 538 insertions(+), 50 deletions(-)
>  create mode 100644 docs/misc/channel.txt
> 
> diff --git a/docs/misc/channel.txt b/docs/misc/channel.txt
> new file mode 100644
> index 0000000..1b8e405
> --- /dev/null
> +++ b/docs/misc/channel.txt
> @@ -0,0 +1,95 @@
> +Xen PV Channels
> +===============
> +
> +A channel is a low-bandwidth private byte stream similar to a serial
> +link. Typical uses of channels are
> +
> +  1. to provide initial configuration information to a VM on boot
> +     (example use: CloudStack's cloud-early-config service)
> +  2. to signal/query an in-guest agent
> +     (example use: oVirt's guest agent)
> +
> +Channels are similar to virtio-serial devices and emulated serial links.
> +Channels are intended to be used in the implementation of libvirt <channel>s
> +when running on Xen.
> +
> +Note: if an application requires a high-bandwidth link then it should use
> +vchan instead.
> +
> +How to use channels: an example
> +-------------------------------
> +
> +Consider a cloud deployment where VMs are cloned from pre-made templates,
> +and customised on first boot by an in-guest agent which sets the IP address,
> +hostname, ssh keys etc. To install the system the cloud administrator would
> +first:
> +
> +  1. Install a guest as normal (no channel configuration necessary)
> +  2. Install the in-guest agent specific to the cloud software. This will
> +     prepare the guest to communicate over the channel, and also prepare
> +     the guest to be cloned safely (sometimes known as "sysprepping")
> +  3. Shutdown the guest
> +  4. Register the guest as a template with the cloud orchestration software
> +  5. Install the cloud orchestration agent in dom0
> +
> +At runtime, when a cloud tenant requests that a VM is created from the template,
> +the sequence of events would be:
> +
> +  1. A VM is "cloned" from the template
> +  2. A unique Unix domain socket path in dom0 is allocated
> +     (e.g. /my/cloud/software/talk/to/domain/<vm uuid>)
> +  3. Domain configuration is created for the VM, listing the channel
> +     name expected by the in-guest agent. In xl syntax this would be:
> +
> +     channel = [ "connection=socket, name=org.my.cloud.software.agent.version1,
> +                  path = /my/cloud/software/talk/to/domain/<vm uuid>" ]
> +
> +  4. The VM is started
> +  5. In dom0 the cloud orchestration agent connects to the Unix domain
> +     socket, writes a handshake message and waits for a reply
> +  6. In the guest, a udev rule (part of the guest agent package) is activated
> +     by the hotplug event, and it starts the in-guest agent.

<scratchies his head> That would mean there must be some form of 
a kernel driver to trigger the hotplug event. Which driver is this?

I am not seeing anything obvious in the hvc_console.c ?

> +  7. The in-guest agent completes a handshake with the dom0 agent
> +  8. The dom0 agent transmits the unique VM configuration: hostname, IP
> +     address, ssh keys etc etc
> +  9. The in-guest agent receives the configuration and applies it.
> +
> +Using channels avoids having to use a temporary disk device or network
> +connection.
> +
> +Design recommendations and pitfalls
> +-----------------------------------
> +
> +It's necessary to install channel-specific software (an "agent") into the guest
> +before you can use a channel. By default a channel will appear as a device
> +which could be mistaken for a serial port or regular console. It is known
> +that some software will proactively seek out serial ports and issue AT commands
> +at them; make sure such software is disabled!
> +
> +Since channels are identified by names, application authors must ensure their
> +channel names are unique to avoid clashes. We recommend that channel names
> +include parts unique to the application such as a domain names. To assist
> +prevent clashes we recommend authors add their names to our global channel
> +registry at the end of this document.
> +
> +Limitations
> +-----------
> +
> +Hotplug and unplug of channels is not currently implemented.
> +
> +Channel name registry
> +---------------------
> +
> +It is important that channel names are globally unique. To help ensure
> +that no-one's name clashes with yours, please add yours to this list.
> +
> +Key:
> +N: Name
> +C: Contact
> +D: Short description of use, possibly including a URL to your software
> +   or API
> +
> +N: org.xenproject.guest.clipboard.0.1
> +C: David Scott <dave.scott@citrix.com>
> +D: Share clipboard data via an in-guest agent. See:
> +   http://wiki.xenproject.org/wiki/Clipboard_sharing_protocol
> diff --git a/docs/misc/console.txt b/docs/misc/console.txt
> index 8a53a95..ed7b795 100644
> --- a/docs/misc/console.txt
> +++ b/docs/misc/console.txt
> @@ -9,10 +9,11 @@ relevant information in xenstore under /local/domain/$DOMID/console.
>  
>  Now many years after the introduction of the pv console we have
>  multiple pv consoles support for pv and hvm guests; multiple pv
> -console backends (qemu and xenconsoled) and emulated serial cards too.
> +console backends (qemu and xenconsoled, see limitations below) and
> +emulated serial cards too.
>  
>  This document tries to describe how the whole system works and how the
> -different components interact with each others.
> +different components interact with each other.
>  
>  The first PV console path in xenstore remains:
>  
> @@ -23,28 +24,63 @@ live in:
>  
>  /local/domain/$DOMID/device/console/$DEVID.
>  
> -The output of a PV console, whether it should be a file, a pty, a
> -socket, or something else, is specified by the toolstack in the xenstore
> -node "output", under the relevant console section.
> -For example:
> +PV consoles have
> +* (optional) string names;
> +* 'connection' information describing to what they should be
> +  connected; and
> +* a 'type' indicating which daemon should process the data.
> +
> +We call a PV console with a name a "channel", in reference to the libvirt
> +concept with the same name and purpose. The file "channels.txt" describes
> +how to use channels and includes a registry of well-known channel names.
> +
> +If the PV console has a name (i.e. it is a "channel") then the name
> +is written to the frontend directory:
> +
> +name = <name>
> +
> +If the PV console has no name (i.e. it is a regular console) then the "name"
> +key is omitted.
> +
> +The toolstack writes 'connection' information in the xenstore backend in
> +the keys
> +* connection: either 'pty' or 'socket'
> +* path: only present if connection = 'socket', the path of the socket to
> +  glue the channel to
> +
> +An artifact of the current implementation, the toolstack will write an
> +extra backend key
> +* output: an identifier only meaningful for qemu/xenconsoled
>  
> -# xenstore-read /local/domain/26/device/console/1/output
> -pty
> +If the toolstack wants the console to be connected to a pty, it will write
> +to the backend:
>  
> -The backend chosen for a particular console is specified by the
> -toolstack in the "type" node on xenstore, under the relevant console
> -section.
> +connection = pty
> +output = pty
> +
> +The backend will write the pty device name to the "tty" node in the
> +console frontend.
> +
> +If the toolstack wants a listening Unix domain socket to be created at path
> +<path>, a connection accepted and data proxied to the console, it will write:
> +
> +connection = socket
> +path = <path>
> +output = chardev:<some internal identifier>
> +
> +where chardev:<some internal identifier> matches a qemu character device
> +configured on the qemu command-line.
> +
> +The backend implementation daemon chosen for a particular console is specified
> +by the toolstack in the "type" node in the xenstore frontend.
>  For example:
>  
>  # xenstore-read /local/domain/26/console/1/type
> -xenconsoled
> +ioemu
>  
>  The supported values are only xenconsoled or ioemu; xenconsoled has
>  several limitations: it can only be used for the first PV console and it
> -can only have a pty as output.
> -
> -If the output is a pty, backends write the device name to the "tty" node
> -in xenstore under the relevant console path.
> +can only connect to a pty.
>  
>  Emulated serials are provided by qemu-dm only to hvm guests; the number
>  of emulated serials depends on how many "-serial" command line options
> @@ -54,7 +90,6 @@ xenstore in the following path:
>  
>  /local/domain/$DOMID/serial/$SERIAL_NUM/tty
>  
> -
>  xenconsole is the tool to connect to a PV console or an emulated serial
>  that has a pty as output. Xenconsole takes a domid as parameter plus an
>  optional console type (pv for PV consoles or serial for emulated
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 77672d0..9ce93d9 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -21,6 +21,15 @@
>  #define PAGE_TO_MEMKB(pages) ((pages) * 4)
>  #define BACKEND_STRING_SIZE 5
>  
> +/* Utility to read backend xenstore keys */
> +#define READ_BACKEND(tgc, subpath) ({                                   \
> +        rc = libxl__xs_read_checked(tgc, XBT_NULL,                      \
> +                                    GCSPRINTF("%s/" subpath, be_path),  \
> +                                    &tmp);                              \
> +        if (rc) goto out;                                               \
> +        (char*)tmp;                                                     \
> +    });
> +
>  int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>                      unsigned flags, xentoollog_logger * lg)
>  {
> @@ -3319,14 +3328,6 @@ static int libxl__device_nic_from_xs_be(libxl__gc *gc,
>  
>      libxl_device_nic_init(nic);
>  
> -#define READ_BACKEND(tgc, subpath) ({                                   \
> -        rc = libxl__xs_read_checked(tgc, XBT_NULL,                      \
> -                                    GCSPRINTF("%s/" subpath, be_path),  \
> -                                    &tmp);                              \
> -        if (rc) goto out;                                               \
> -        (char*)tmp;                                                     \
> -    });
> -
>      tmp = READ_BACKEND(gc, "handle");
>      if (tmp)
>          nic->devid = atoi(tmp);
> @@ -3506,28 +3507,33 @@ const char *libxl__device_nic_devname(libxl__gc *gc,
>  /******************************************************************************/
>  int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>                                libxl__device_console *console,
> -                              libxl__domain_build_state *state)
> +                              libxl__domain_build_state *state,
> +                              libxl__device *device)
>  {
>      flexarray_t *front, *ro_front;
>      flexarray_t *back;
> -    libxl__device device;
>      int rc;
>  
>      if (console->devid && state) {
>          rc = ERROR_INVAL;
>          goto out;
>      }
> +    if (!console->devid && (console->name || console->path)) {
> +        LOG(ERROR, "Primary console has invalid configuration");
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
>  
>      front = flexarray_make(gc, 16, 1);
>      ro_front = flexarray_make(gc, 16, 1);
>      back = flexarray_make(gc, 16, 1);
>  
> -    device.backend_devid = console->devid;
> -    device.backend_domid = console->backend_domid;
> -    device.backend_kind = LIBXL__DEVICE_KIND_CONSOLE;
> -    device.devid = console->devid;
> -    device.domid = domid;
> -    device.kind = LIBXL__DEVICE_KIND_CONSOLE;
> +    device->backend_devid = console->devid;
> +    device->backend_domid = console->backend_domid;
> +    device->backend_kind = LIBXL__DEVICE_KIND_CONSOLE;
> +    device->devid = console->devid;
> +    device->domid = domid;
> +    device->kind = LIBXL__DEVICE_KIND_CONSOLE;
>  
>      flexarray_append(back, "frontend-id");
>      flexarray_append(back, libxl__sprintf(gc, "%d", domid));
> @@ -3540,6 +3546,19 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>      flexarray_append(back, "protocol");
>      flexarray_append(back, LIBXL_XENCONSOLE_PROTOCOL);
>  
> +    if (console->name) {
> +        flexarray_append(ro_front, "name");
> +        flexarray_append(ro_front, console->name);
> +    }
> +    if (console->connection) {
> +        flexarray_append(back, "connection");
> +        flexarray_append(back, console->connection);
> +    }
> +    if (console->path) {
> +        flexarray_append(back, "path");
> +        flexarray_append(back, console->path);
> +    }
> +
>      flexarray_append(front, "backend-id");
>      flexarray_append(front, libxl__sprintf(gc, "%d", console->backend_domid));
>  
> @@ -3566,8 +3585,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>          flexarray_append(front, "protocol");
>          flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
>      }
> -
> -    libxl__device_generic_add(gc, XBT_NULL, &device,
> +    libxl__device_generic_add(gc, XBT_NULL, device,
>                                libxl__xs_kvs_of_flexarray(gc, back, back->count),
>                                libxl__xs_kvs_of_flexarray(gc, front, front->count),
>                                libxl__xs_kvs_of_flexarray(gc, ro_front, ro_front->count));
> @@ -3578,6 +3596,216 @@ out:
>  
>  /******************************************************************************/
>  
> +int libxl__init_console_from_channel(libxl__gc *gc,
> +                                     libxl__device_console *console,
> +                                     int dev_num,
> +                                     libxl_device_channel *channel)
> +{
> +    int rc;

Newline here please.

> +    libxl__device_console_init(console);
> +    console->devid = dev_num;
> +    console->consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> +    if (!channel->name){

Missing space before '{'

> +        LOG(ERROR, "channel %d has no name", channel->devid);
> +        return ERROR_INVAL;
> +    }
> +    console->name = libxl__strdup(NOGC, channel->name);
> +
> +    if (channel->backend_domname) {
> +        rc = libxl_domain_qualifier_to_domid(CTX, channel->backend_domname,
> +                                             &channel->backend_domid);
> +        if (rc < 0) return rc;

Don't want to free 'console->name' ? Especially as you used the NOGC variant?

> +    }
> +
> +    console->backend_domid = channel->backend_domid;
> +
> +    /* The xenstore 'output' node tells the backend what to connect the console
> +       to. If the channel has "connection = pty" then the "output" node will be
> +       set to "pty". If the channel has "connection = socket" then the "output"
> +       node will be set to "chardev:libxl-channel%d". This tells the qemu
> +       backend to proxy data between the console ring and the character device
> +       with id "libxl-channel%d". These character devices are currently defined
> +       on the qemu command-line via "-chardev" options in libxl_dm.c */
> +
> +    switch (channel->connection) {
> +        case LIBXL_CHANNEL_CONNECTION_UNKNOWN:
> +            LOG(ERROR, "channel %d has no defined connection; "
> +                "to where should it be connected?", channel->devid);

Ditto? Don't want to free the 'console->name'?

> +            return ERROR_INVAL;
> +        case LIBXL_CHANNEL_CONNECTION_PTY:
> +            console->connection = libxl__strdup(NOGC, "pty");
> +            console->output = libxl__sprintf(NOGC, "pty");
> +            break;
> +        case LIBXL_CHANNEL_CONNECTION_SOCKET:
> +            console->connection = libxl__strdup(NOGC, "socket");

How about that being done after the 'if (..)' clause below?
> +            if (!channel->u.socket.path) {
> +                LOG(ERROR, "channel %d has no path", channel->devid);
> +                return ERROR_INVAL;
> +            }
> +            console->path = libxl__strdup(NOGC, channel->u.socket.path);
> +            console->output = libxl__sprintf(NOGC, "chardev:libxl-channel%d",
> +                                             channel->devid);
> +            break;
> +        default:
> +            /* We've forgotten to add the clause */
> +            LOG(ERROR, "%s: missing implementation for channel connection %d",
> +                __func__, channel->connection);
> +            abort();
> +    }
> +
> +    return 0;
> +}
> +
> +static int libxl__device_channel_from_xs_be(libxl__gc *gc,
> +                                            const char *be_path,
> +                                            libxl_device_channel *channel)
> +{
> +    const char *tmp;
> +    int rc;
> +
> +    libxl_device_channel_init(channel);
> +
> +    /* READ_BACKEND is from libxl__device_nic_from_xs_be above */
> +    channel->name = READ_BACKEND(NOGC, "name");
> +    tmp = READ_BACKEND(gc, "connection");
> +    if (!strcmp(tmp, "pty")) {
> +        channel->connection = LIBXL_CHANNEL_CONNECTION_PTY;
> +    } else if (!strcmp(tmp, "socket")) {
> +        channel->connection = LIBXL_CHANNEL_CONNECTION_SOCKET;
> +        channel->u.socket.path = READ_BACKEND(NOGC, "path");
> +    } else {
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    rc = 0;
> + out:
> +    return rc;
> +}
> +
> +static int libxl__append_channel_list_of_type(libxl__gc *gc,
> +                                              uint32_t domid,
> +                                              const char *type,
> +                                              libxl_device_channel **channels,
> +                                              int *nchannels)
> +{
> +    char *fe_path = NULL, *be_path = NULL;
> +    char **dir = NULL;
> +    unsigned int n = 0, devid = 0;
> +    libxl_device_channel *next = NULL;
> +    int rc = 0, i;
> +
> +    fe_path = GCSPRINTF("%s/device/%s",
> +                        libxl__xs_get_dompath(gc, domid), type);
> +    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &n);
> +    if (!dir || !n)
> +      goto out;
> +
> +    for (i = 0; i < n; i++) {
> +        const char *p, *name;
> +        libxl_device_channel *tmp;

Newline missing here.
> +        p = libxl__sprintf(gc, "%s/%s", fe_path, dir[i]);
> +        name = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/name", p));
> +        /* 'channels' are consoles with names, so ignore all consoles
> +           without names */
> +        if (!name) continue;
> +        be_path = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend", p));
> +        tmp = realloc(*channels,
> +                      sizeof(libxl_device_channel) * (*nchannels + devid + 1));
> +        if (!tmp) {
> +          rc = ERROR_NOMEM;
> +          goto out;
> +        }
> +        *channels = tmp;
> +        next = *channels + *nchannels + devid;
> +        rc = libxl__device_channel_from_xs_be(gc, be_path, next);
> +        if (rc) goto out;
> +        next->devid = devid;
> +        devid++;
> +    }
> +    *nchannels += devid;
> +    return 0;
> +
> + out:
> +    return rc;
> +}
> +
> +libxl_device_channel *libxl_device_channel_list(libxl_ctx *ctx,
> +                                                uint32_t domid,
> +                                                int *num)
> +{
> +    GC_INIT(ctx);
> +    libxl_device_channel *channels = NULL;
> +    int rc;
> +
> +    *num = 0;
> +
> +    rc = libxl__append_channel_list_of_type(gc, domid, "console", &channels, num);
> +    if (rc) goto out_err;
> +
> +    GC_FREE;
> +    return channels;
> +
> +out_err:
> +    LOG(ERROR, "Unable to list channels");
> +    while (*num) {
> +        (*num)--;
> +        libxl_device_channel_dispose(&channels[*num]);
> +    }
> +    free(channels);
> +    return NULL;
> +}
> +
> +int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
> +                                 libxl_device_channel *channel,
> +                                 libxl_channelinfo *channelinfo)
> +{
> +    GC_INIT(ctx);
> +    char *dompath, *fe_path;
> +    char *val;
> +
> +    dompath = libxl__xs_get_dompath(gc, domid);
> +    channelinfo->devid = channel->devid;
> +
> +    fe_path = libxl__sprintf(gc, "%s/device/console/%d", dompath,
> +                             channelinfo->devid + 1);
> +    channelinfo->backend = xs_read(ctx->xsh, XBT_NULL,
> +                                   libxl__sprintf(gc, "%s/backend",
> +                                   fe_path), NULL);
> +    if (!channelinfo->backend) {
> +        GC_FREE;
> +        return ERROR_FAIL;
> +    }
> +    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend-id", fe_path));
> +    channelinfo->backend_id = val ? strtoul(val, NULL, 10) : -1;
> +    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/state", fe_path));
> +    channelinfo->state = val ? strtoul(val, NULL, 10) : -1;
> +    channelinfo->frontend = xs_read(ctx->xsh, XBT_NULL,
> +                                    GCSPRINTF("%s/frontend",
> +                                    channelinfo->backend), NULL);
> +    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/frontend-id",
> +                         channelinfo->backend));
> +    channelinfo->frontend_id = val ? strtoul(val, NULL, 10) : -1;
> +    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/ring-ref", fe_path));
> +    channelinfo->rref = val ? strtoul(val, NULL, 10) : -1;
> +    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/port", fe_path));
> +    channelinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
> +
> +    channelinfo->connection = channel->connection;
> +    switch (channel->connection) {
> +         case LIBXL_CHANNEL_CONNECTION_PTY:
> +             val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/tty", fe_path));
> +             channelinfo->u.pty.path = strdup(val);
> +             break;
> +         default:
> +             break;
> +    }
> +    GC_FREE;
> +    return 0;
> +}
> +
> +/******************************************************************************/
> +
>  int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb)
>  {
>      int rc;
> @@ -3842,6 +4070,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
>  DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
>  DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
>  
> +/* channel/console hotunplug is not implemented. There are 2 possibilities:
> + * 1. add support for secondary consoles to xenconsoled
> + * 2. dynamically add/remove qemu chardevs via qmp messages. */
> +
>  #undef DEFINE_DEVICE_REMOVE
>  
>  /******************************************************************************/
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index bc68cac..300e489 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -583,6 +583,15 @@ typedef struct libxl__ctx libxl_ctx;
>   */
>  #define LIBXL_HAVE_BUILDINFO_KERNEL 1
>  
> +/*
> + * LIBXL_HAVE_DEVICE_CHANNEL
> + *
> + * If this is defined, then the libxl_device_channel struct exists
> + * and channels can be attached to a domain. Channels manifest as consoles
> + * with names, see docs/misc/console.txt.
> + */
> +#define LIBXL_HAVE_DEVICE_CHANNEL 1
> +
>  /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
>   * called from within libxl itself. Callers outside libxl, who
>   * do not #include libxl_internal.h, are fine. */
> @@ -1129,6 +1138,17 @@ libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num
>  int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t domid,
>                                libxl_device_nic *nic, libxl_nicinfo *nicinfo);
>  
> +/*
> + * Virtual Channels
> + * Channels manifest as consoles with names, see docs/misc/channels.txt
> + */
> +libxl_device_channel *libxl_device_channel_list(libxl_ctx *ctx,
> +                                                uint32_t domid,
> +                                                int *num);
> +int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
> +                                 libxl_device_channel *channel,
> +                                 libxl_channelinfo *channelinfo);
> +
>  /* Virtual TPMs */
>  int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm *vtpm,
>                            const libxl_asyncop_how *ao_how)
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 8b82584..4be4c5c 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -387,14 +387,16 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>      return 0;
>  }
>  
> -static int init_console_info(libxl__device_console *console, int dev_num)
> +static int init_console_info(libxl__gc *gc,
> +                             libxl__device_console *console,
> +                             int dev_num)
>  {
> -    memset(console, 0x00, sizeof(libxl__device_console));
> +    libxl__device_console_init(console);

How come?
Should that be a seperate patch - to use 'libxl__device_console_init' ?

>      console->devid = dev_num;
>      console->consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
> -    console->output = strdup("pty");
> -    if (!console->output)
> -        return ERROR_NOMEM;
> +    console->output = libxl__strdup(NOGC, "pty");
> +    /* console->{name,connection,path} are NULL on normal consoles.
> +       Only 'channels' when mapped to consoles have a string name. */

And the 'return ERROR_NOMEM' is gone too?
>      return 0;

Why don't you just make this function 'void'?
>  }
>  
> @@ -1194,17 +1196,31 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>          }
>      }
>  
> +    /* For both HVM and PV the 0th console is a regular console. We
> +       map channels to IOEMU consoles starting at 1 */
> +    for (i = 0; i < d_config->num_channels; i++) {
> +        libxl__device_console console;
> +        libxl__device device;
> +        ret = libxl__init_console_from_channel(gc, &console, i + 1,
> +                                               &d_config->channels[i]);
> +        if ( ret )
> +            goto error_out;

And since 'console' is on the stack, and we have potentially allocated
'->name', now we are leaking '->name' when we get to error_out (as we
haven't called 'libxl__device_console_dispose(&console)'.

> +        libxl__device_console_add(gc, domid, &console, NULL, &device);
> +        libxl__device_console_dispose(&console);
> +    }
> +
>      switch (d_config->c_info.type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
>      {
>          libxl__device_console console;
> +        libxl__device device;
>          libxl_device_vkb vkb;
>  
> -        ret = init_console_info(&console, 0);
> +        ret = init_console_info(gc, &console, 0);
>          if ( ret )
>              goto error_out;
>          console.backend_domid = state->console_domid;
> -        libxl__device_console_add(gc, domid, &console, state);
> +        libxl__device_console_add(gc, domid, &console, state, &device);
>          libxl__device_console_dispose(&console);
>  
>          libxl_device_vkb_init(&vkb);
> @@ -1231,22 +1247,24 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>      {
>          int need_qemu = 0;
>          libxl__device_console console;
> +        libxl__device device;
>  
>          for (i = 0; i < d_config->num_vfbs; i++) {
>              libxl__device_vfb_add(gc, domid, &d_config->vfbs[i]);
>              libxl__device_vkb_add(gc, domid, &d_config->vkbs[i]);
>          }
>  
> -        ret = init_console_info(&console, 0);
> +        ret = init_console_info(gc, &console, 0);
>          if ( ret )
>              goto error_out;
>  
>          need_qemu = libxl__need_xenpv_qemu(gc, 1, &console,
>                  d_config->num_vfbs, d_config->vfbs,
> -                d_config->num_disks, &d_config->disks[0]);
> +                d_config->num_disks, &d_config->disks[0],
> +                d_config->num_channels, &d_config->channels[0]);
>  
>          console.backend_domid = state->console_domid;
> -        libxl__device_console_add(gc, domid, &console, state);
> +        libxl__device_console_add(gc, domid, &console, state, &device);
>          libxl__device_console_dispose(&console);
>  
>          if (need_qemu) {
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index fbc82fd..dc748be 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -416,8 +416,9 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>      const libxl_sdl_info *sdl = dm_sdl(guest_config);
>      const char *keymap = dm_keymap(guest_config);
>      flexarray_t *dm_args;
> -    int i;
> +    int i, connection, devid;
>      uint64_t ram_size;
> +    const char *path, *chardev;
>  
>      dm_args = flexarray_make(gc, 16, 1);
>  
> @@ -434,6 +435,28 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>      flexarray_append(dm_args, "-mon");
>      flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
>  
> +    for (i = 0; i < guest_config->num_channels; i++) {
> +        connection = guest_config->channels[i].connection;
> +        devid = guest_config->channels[i].devid;
> +        switch (connection) {
> +            case LIBXL_CHANNEL_CONNECTION_PTY:
> +                chardev = GCSPRINTF("pty,id=libxl-channel%d", devid);
> +                break;
> +            case LIBXL_CHANNEL_CONNECTION_SOCKET:
> +                path = guest_config->channels[i].u.socket.path;
> +                chardev = GCSPRINTF("socket,id=libxl-channel%d,path=%s,"
> +                                    "server,nowait", devid, path);
> +                break;
> +            default:
> +                /* We've forgotten to add the clause */
> +                LOG(ERROR, "%s: unknown channel connection %d",
> +                    __func__, connection);
> +                return NULL;
> +        }
> +        flexarray_append(dm_args, "-chardev");
> +        flexarray_append(dm_args, (void*)chardev);
> +    }
> +
>      /*
>       * Remove default devices created by qemu. Qemu will create only devices
>       * defined by xen, since the devices not defined by xen are not usable.
> @@ -1116,6 +1139,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
>      }
>  
>      for (i = 0; i < num_console; i++) {
> +        libxl__device device;
>          console[i].devid = i;
>          console[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
>          /* STUBDOM_CONSOLE_LOGGING (console 0) is for minios logging
> @@ -1146,7 +1170,8 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
>                  break;
>          }
>          ret = libxl__device_console_add(gc, dm_domid, &console[i],
> -                        i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL);
> +                        i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL,
> +                        &device);
>          if (ret)
>              goto out;
>      }
> @@ -1566,7 +1591,8 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
>  int libxl__need_xenpv_qemu(libxl__gc *gc,
>          int nr_consoles, libxl__device_console *consoles,
>          int nr_vfbs, libxl_device_vfb *vfbs,
> -        int nr_disks, libxl_device_disk *disks)
> +        int nr_disks, libxl_device_disk *disks,
> +        int nr_channels, libxl_device_channel *channels)
>  {
>      int i, ret = 0;
>      uint32_t domid;
> @@ -1606,6 +1632,20 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
>          }
>      }
>  
> +    if (nr_channels > 0) {
> +        ret = libxl__get_domid(gc, &domid);
> +        if (ret) goto out;
> +        for (i = 0; i < nr_channels; i++) {
> +            if (channels[i].backend_domid == domid) {
> +                /* xenconsoled is limited to the first console only.
> +                   Until this restriction is removed we must use qemu for
> +                   secondary consoles which includes all channels. */
> +                ret = 1;
> +                goto out;
> +            }
> +        }
> +    }
> +
>  out:
>      return ret;
>  }
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index f61673c..151c474 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1033,7 +1033,8 @@ _hidden int libxl__device_disk_dev_number(const char *virtpath,
>  
>  _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>                                        libxl__device_console *console,
> -                                      libxl__domain_build_state *state);
> +                                      libxl__domain_build_state *state,
> +                                      libxl__device *device);
>  
>  /* Returns 1 if device exists, 0 if not, ERROR_* (<0) on error. */
>  _hidden int libxl__device_exists(libxl__gc *gc, xs_transaction_t t,
> @@ -1049,6 +1050,10 @@ _hidden int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
>                                      const char *state);
>  _hidden int libxl__nic_type(libxl__gc *gc, libxl__device *dev,
>                              libxl_nic_type *nictype);
> +_hidden int libxl__init_console_from_channel(libxl__gc *gc,
> +                                             libxl__device_console *console,
> +                                             int dev_num,
> +                                             libxl_device_channel *channel);
>  
>  /*
>   * For each aggregate type which can be used as an input we provide:
> @@ -1468,7 +1473,8 @@ _hidden const char *libxl__domain_device_model(libxl__gc *gc,
>  _hidden int libxl__need_xenpv_qemu(libxl__gc *gc,
>          int nr_consoles, libxl__device_console *consoles,
>          int nr_vfbs, libxl_device_vfb *vfbs,
> -        int nr_disks, libxl_device_disk *disks);
> +        int nr_disks, libxl_device_disk *disks,
> +        int nr_channels, libxl_device_channel *channels);
>  
>  /*
>   * This function will cause the whole libxl process to hang
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index f1fcbc3..59c3d0b 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -69,6 +69,12 @@ libxl_domain_type = Enumeration("domain_type", [
>      (2, "PV"),
>      ], init_val = "LIBXL_DOMAIN_TYPE_INVALID")
>  
> +libxl_channel_connection = Enumeration("channel_connection", [
> +    (0, "UNKNOWN"),
> +    (1, "PTY"),
> +    (2, "SOCKET"), # a listening Unix domain socket
> +    ])
> +
>  libxl_device_model_version = Enumeration("device_model_version", [
>      (0, "UNKNOWN"),
>      (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
> @@ -269,6 +275,22 @@ libxl_cpupoolinfo = Struct("cpupoolinfo", [
>      ("cpumap",      libxl_bitmap)
>      ], dir=DIR_OUT)
>  
> +libxl_channelinfo = Struct("channelinfo", [
> +    ("backend", string),
> +    ("backend_id", uint32),
> +    ("frontend", string),
> +    ("frontend_id", uint32),
> +    ("devid", libxl_devid),
> +    ("state", integer),
> +    ("evtch", integer),
> +    ("rref", integer),
> +    ("u", KeyedUnion(None, libxl_channel_connection, "connection",
> +           [("unknown", None),
> +            ("pty", Struct(None, [("path", string),])),
> +            ("socket", None),
> +           ])),
> +    ], dir=DIR_OUT)
> +
>  libxl_vminfo = Struct("vminfo", [
>      ("uuid", libxl_uuid),
>      ("domid", libxl_domid),
> @@ -491,6 +513,18 @@ libxl_device_vtpm = Struct("device_vtpm", [
>      ("uuid",             libxl_uuid),
>  ])
>  
> +libxl_device_channel = Struct("device_channel", [
> +    ("backend_domid", libxl_domid),
> +    ("backend_domname", string),
> +    ("devid", libxl_devid),
> +    ("name", string),
> +    ("u", KeyedUnion(None, libxl_channel_connection, "connection",
> +           [("unknown", None),
> +            ("pty", None),
> +            ("socket", Struct(None, [("path", string)])),
> +           ])),
> +])
> +
>  libxl_domain_config = Struct("domain_config", [
>      ("c_info", libxl_domain_create_info),
>      ("b_info", libxl_domain_build_info),
> @@ -501,6 +535,9 @@ libxl_domain_config = Struct("domain_config", [
>      ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
>      ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
>      ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
> +    # a channel manifests as a console with a name,
> +    # see docs/misc/channels.txt
> +    ("channels", Array(libxl_device_channel, "num_channels")),
>  
>      ("on_poweroff", libxl_action_on_shutdown),
>      ("on_reboot", libxl_action_on_shutdown),
> diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
> index 800361b..5e55685 100644
> --- a/tools/libxl/libxl_types_internal.idl
> +++ b/tools/libxl/libxl_types_internal.idl
> @@ -34,6 +34,11 @@ libxl__device_console = Struct("device_console", [
>      ("devid", integer),
>      ("consback", libxl__console_backend),
>      ("output", string),
> +    # A regular console has no name.
> +    # A console with a name is called a 'channel', see docs/misc/channels.txt
> +    ("name", string),
> +    ("connection", string),
> +    ("path", string),
>      ])
>  
>  libxl__device_action = Enumeration("device_action", [
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel