[PATCH for-4.5 v5] add support for libvirt-like channels

[PATCH for-4.5 v5] add support for libvirt-like channels

[David Scott]

Hi,

I've incorporated IanC's feedback from the v4. IanJ: I've tried to
incorporate your feedback from the earlier v3 -- let me know what you think.
 
The blurb:

Several libvirt applications (e.g. oVirt, CloudStack) make use of 'channels': 
low-bandwidth private host <-> guest communication links which resemble serial 
ports. Typical uses include: 

* initial VM configuration without using the network: read the config data 
directly from the channel on boot 

* controlling a guest agent: signal shutdown reqests, exchange clipboard data, 
trigger resolution changes 

This patch set re-uses the existing PV console protocol implemented by qemu 
to provide this service. 

If you declare a channel in your .xl file as follows: 

channel = [ "connection=socket,path=/tmp/mysock,name=org.mydomain.guest-agent" ] 

then an extra PV console will be added to your guest. This console has the 
extra key in the frontend 

name = guest-agent 

which allows udev scripts in the VM to create a named device in a well-known 
location (e.g. /dev/xen-channels/guest-agent, similar to the KVM /dev/vports). 
The qemu process in the backend domain will proxy the data to/from the named 
Unix domain socket (in this case /tmp/mysock). 

Note this mechanism is intended for low-bandwidth communication only. If an 
application requires a high-bandwith connection then it should use a direct 
vchan connection (and not proxy it via a qemu). 

Changes since v4:
* doc: highlight that the 'output' key in the console configuration
  should be considered an internal implementation artifact
* return EINVAL if a primary console has invalid configuration
* Use LOG(ERROR,... rather than LIBXL__LOG(CTX, LIBXL__LOG_ERROR
* Use an abort() when implementation code is missing
* move READ_BACKEND to the top of the file (since it's generally useful)
* Remove stray tab

Changes since v3:
* docs: coalesce the docs patch into the libxl patch
* docs: in channels.txt give high-level usage information, pitfalls and
        channgle registry
* docs: move the xenstore paths into the existing console.txt, which is
        referenced from xenstore-paths.markdown
* idl: rename 'kind' to 'connection'
* xl: add parser functions for comma-separated lists of pairs

Changes since v2:
* trim down the 'kinds' of channels to PTY and SOCKET -- these seem the most
 useful and we can add more later
* add a channelinfo (queryable by 'channel-list') to check the state of each
 channel, and for a kind=PTY discover the slave device path
* IDL: switched to KeyedUnion for both the channel and channelinfo since
 each 'kind' will have different parameters (e.g. only SOCKET has PATH)
* write all the backend configuration parameters to xenstore -- where we were
 using qemu -chardev some crucial information was only on the command-line.
* add LIBXL_HAVE_DEVICE_CHANNEL
* docs: replace 'should' with 'will' e.g. the backend will be connected to
 a Unix domain socket
* squash all the libxl patches together into a coherent whole
* explain that there is no registry of channel names and so people should use
 unique information to create them (e.g. include domain name and interface
 version)

Changes since v1: 
* extend xl.cfg(5) 
* add docs/misc/channel.txt 
* libxl_types.idl: omit unnecessary init_val = 0 
* xl_cmdimpl.c: fixed over-long lines 
* xl_cmdimpl.c: use xrealloc (via ARRAY_EXTEND_INIT) 
* xl_cmdimpl.c: exit() on parse failure rather than ignore configuration 
* libxl_create.c: use libxl__device_console_init instead of memset 
* libxl_create.c: use libxl__strdup(NOGC rather than raw strdup 
* libxl.c: add name=<name> to console frontend 
* libxl.c: resolve the backend_domid from backend_domname 
* libxl_dm.c: channels[i].devid rather than i 
* libxl_dm.c: fix indentation 
* libxl_dm.c: use GCSPRINTF convenience macro 

Signed-off-by: David Scott <dave.scott@citrix.com>

[PATCH v5 for-4.5 1/2] libxl: add support for 'channels'

[David Scott]

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                |   61 +++++---
 tools/libxl/libxl.c                  |  268 +++++++++++++++++++++++++++++++---
 tools/libxl/libxl.h                  |   17 +++
 tools/libxl/libxl_create.c           |   38 +++--
 tools/libxl/libxl_dm.c               |   46 +++++-
 tools/libxl/libxl_internal.h         |   10 +-
 tools/libxl/libxl_types.idl          |   35 +++++
 tools/libxl/libxl_types_internal.idl |    4 +
 9 files changed, 524 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.
+  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..3ecfda4 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,55 @@ 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.
+
+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
+
+If the toolstack wants the console to be connected to a pty, it will write
+to the backend:
+
+connection = pty
+output = pty
 
-# xenstore-read /local/domain/26/device/console/1/output
-pty
+The backend will write the pty device name to the "tty" node in the
+console frontend.
 
-The backend chosen for a particular console is specified by the
-toolstack in the "type" node on xenstore, under the relevant console
-section.
+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 +82,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 2ae5fca..b0af320 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)
 {
@@ -3085,14 +3094,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);
@@ -3266,28 +3267,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));
@@ -3300,6 +3306,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));
 
@@ -3326,8 +3345,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));
@@ -3338,6 +3356,216 @@ out:
 
 /******************************************************************************/
 
+int libxl__init_console_from_channel(libxl__gc *gc,
+                                     libxl__device_console *console,
+                                     int dev_num,
+                                     libxl_device_channel *channel)
+{
+    int rc;
+    libxl__device_console_init(console);
+    console->devid = dev_num;
+    console->consback = LIBXL__CONSOLE_BACKEND_IOEMU;
+    if (!channel->name){
+        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;
+    }
+
+    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);
+            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");
+            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;
+        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;
@@ -3602,6 +3830,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 460207b..0d19aa7 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -568,6 +568,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 both at build-time and at
+ * run-time by hotplug.
+ */
+#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. */
@@ -1101,6 +1110,14 @@ 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 */
+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 ee328e9..c3f1c25 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);
     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. */
     return 0;
 }
 
@@ -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;
+        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);
@@ -1222,22 +1238,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 103cbca..84c9b41 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.
@@ -1109,6 +1132,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
@@ -1139,7 +1163,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;
     }
@@ -1559,7 +1584,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;
@@ -1599,6 +1625,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 04c9378..656bf7a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1017,7 +1017,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);
 
 _hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
         libxl__device *device, char **bents, char **fents, char **ro_fents);
@@ -1030,6 +1031,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:
@@ -1449,7 +1454,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 931c9e9..dab65ee 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -66,6 +66,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)
@@ -266,6 +272,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),
@@ -488,6 +510,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),
@@ -498,6 +532,7 @@ 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")),
+    ("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..a62167c 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -34,6 +34,10 @@ libxl__device_console = Struct("device_console", [
     ("devid", integer),
     ("consback", libxl__console_backend),
     ("output", string),
+    # These are specific to the 'channels', built on top of consoles:
+    ("name", string),
+    ("connection", string),
+    ("path", string),
     ])
 
 libxl__device_action = Enumeration("device_action", [
-- 
1.7.10.4

[PATCH v5 for-4.5 2/2] xl: add support for 'channels'

[David Scott]

This adds support for channel declarations of the form:

 channel = [ "name=...,connection=...[,path=...][,backend=...]" ]

where 'name' is a label to identify the channel to the frontend.

If 'connection = pty' then the channel is connected to a pty in the
 backend domain
If 'connection = socket' then the channel is connected to a Unix domain
 socket given by 'path = ...' in the backend domain.

This patch also adds the command:

 xl channel-list <domain>

which allows the state of channels to be queried. In particular if
'connection=pty' this will show the path of the pty slave device.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 docs/man/xl.cfg.pod.5     |   51 +++++++++++
 docs/man/xl.pod.1         |   10 +++
 tools/libxl/xl.h          |    1 +
 tools/libxl/xl_cmdimpl.c  |  207 ++++++++++++++++++++++++++++++++++++++++++---
 tools/libxl/xl_cmdtable.c |    5 ++
 5 files changed, 264 insertions(+), 10 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 517ae2f..cf7b790 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -530,6 +530,57 @@ L<qemu(1)> manpage. The default is B<en-us>.
 
 =back
 
+=item B<channel=[ "CHANNEL_SPEC_STRING", "CHANNEL_SPEC_STRING", ...]>
+
+Specifies the virtual channels to be provided to the guest. A
+channel is a low-bandwidth, bidirectional byte stream, which resembles
+a serial link. Typical uses for channels include transmitting VM
+configuration after boot and signalling to in-guest agents. Please see
+F<docs/misc/channels.txt> for more details.
+
+Each B<CHANNEL_SPEC_STRING> is a comma-separated list of C<KEY=VALUE>
+settings, from the following list:
+
+=over 4
+
+=item C<backend=DOMAIN>
+
+Specify the backend domain name or id. This parameter is optional. If
+this parameter is omitted then the toolstack domain will be assumed.
+
+=item C<name=NAME>
+
+Specify the string name for this device. This parameter is mandatory.
+This should be a well-known name for the specific application (e.g.
+guest agent) and should be used by the frontend to connect the
+application to the right channel device. There is no formal registry
+of channel names, so application authors are encouraged to make their
+names unique by including domain name and version number in the string
+(e.g. org.mydomain.guestagent.1).
+
+=item C<connection=CONNECTION>
+
+Specify how the backend will be implemented. This following options are
+available:
+
+=over 4
+
+=item B<connection=SOCKET>:
+
+The backend will bind a Unix domain socket (at the path given by
+B<path=PATH>), call listen and accept connections. The backend will proxy
+data between the channel and the connected socket.
+
+=item B<connection=PTY>:
+
+The backend will create a pty and proxy data between the channel and the
+master device. The command B<xl channel-list> can be used to discover the
+assigned slave device.
+
+=back
+
+=back
+
 =item B<pci=[ "PCI_SPEC_STRING", "PCI_SPEC_STRING", ... ]>
 
 Specifies the host PCI devices to passthrough to this guest. Each B<PCI_SPEC_STRING>
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 9d1c2a5..b8a3d0e 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1210,6 +1210,16 @@ List virtual network interfaces for a domain.
 
 =back
 
+=head2 CHANNEL DEVICES
+
+=over 4
+
+=item B<channel-list> I<domain-id>
+
+List virtual channel interfaces for a domain.
+
+=back
+
 =head2 VTPM DEVICES
 
 =over 4
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 10a2e66..afc5f8b 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -78,6 +78,7 @@ int main_top(int argc, char **argv);
 int main_networkattach(int argc, char **argv);
 int main_networklist(int argc, char **argv);
 int main_networkdetach(int argc, char **argv);
+int main_channellist(int argc, char **argv);
 int main_blockattach(int argc, char **argv);
 int main_blocklist(int argc, char **argv);
 int main_blockdetach(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e6b9615..f97b429 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -298,6 +298,18 @@ static void *xrealloc(void *ptr, size_t sz) {
     return r;
 }
 
+static char *xstrdup(const char *x)
+{
+    char *r;
+    r = strdup(x);
+    if (!r) {
+        fprintf(stderr, "xl: Unable to strdup a string of length %zu.\n",
+                strlen(x));
+        exit(-ERROR_FAIL);
+    }
+    return r;
+}
+
 #define ARRAY_EXTEND_INIT(array,count,initfn)                           \
     ({                                                                  \
         typeof((count)) array_extend_old_count = (count);               \
@@ -516,7 +528,7 @@ static void split_string_into_string_list(const char *str,
 
     s = strdup(str);
     if (s == NULL) {
-        fprintf(stderr, "unable to allocate memory to parse bootloader args\n");
+        fprintf(stderr, "unable to allocate memory to split string\n");
         exit(-1);
     }
 
@@ -532,7 +544,7 @@ static void split_string_into_string_list(const char *str,
 
     sl = malloc((nr+1) * sizeof (char *));
     if (sl == NULL) {
-        fprintf(stderr, "unable to allocate memory for bootloader args\n");
+        fprintf(stderr, "unable to allocate memory to split string\n");
         exit(-1);
     }
 
@@ -549,6 +561,64 @@ static void split_string_into_string_list(const char *str,
     free(s);
 }
 
+typedef int (*char_predicate_t)(const int c);
+
+static void trim(char_predicate_t predicate, const char *input, char **output)
+{
+    char *p, *q, *tmp;
+    if (*input == '\000')
+        return;
+    /* Input has length >= 1 */
+
+    p = tmp = xstrdup(input);
+    /* Skip past the first whitespace */
+    while ((*p != '\000') && (predicate(*p)))
+        p ++;
+    q = p + strlen(p) - 1;
+    /* q points to the last non-NULL character */
+    while ((q > p) && (predicate(*q)))
+        q --;
+    /* q points to the last character we want */
+    q ++;
+    *q = '\000';
+    *output = xstrdup(p);
+    free(tmp);
+}
+
+static int split_string_into_pair(const char *str,
+                                  const char *delim,
+                                  char **a,
+                                  char **b)
+{
+    char *s, *p, *saveptr, *aa = NULL, *bb = NULL;
+    int rc = 0;
+
+    s = xstrdup(str);
+
+    p = strtok_r(s, delim, &saveptr);
+    if (p == NULL) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+    aa = xstrdup(p);
+    p = strtok_r(NULL, delim, &saveptr);
+    if (p == NULL) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+    bb = xstrdup(p);
+
+    *a = aa;
+    aa = NULL;
+    *b = bb;
+    bb = NULL;
+out:
+    free(s);
+    free(aa);
+    free(bb);
+    return rc;
+}
+
 static int parse_range(const char *str, unsigned long *a, unsigned long *b)
 {
     const char *nstr;
@@ -797,6 +867,12 @@ static void parse_vcpu_affinity(libxl_domain_build_info *b_info,
     }
 }
 
+static void replace_string(char **str, const char *val)
+{
+    free(*str);
+    *str = strdup(val);
+}
+
 static void parse_config_data(const char *config_source,
                               const char *config_data,
                               int config_len,
@@ -806,7 +882,7 @@ static void parse_config_data(const char *config_source,
     long l;
     XLU_Config *config;
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
-    XLU_ConfigList *ioports, *irqs, *iomem;
+    XLU_ConfigList *channels, *ioports, *irqs, *iomem;
     int num_ioports, num_irqs, num_iomem, num_cpus;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 0;
@@ -1291,6 +1367,79 @@ static void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_list (config, "channel", &channels, 0, 0)) {
+        d_config->num_channels = 0;
+        d_config->channels = NULL;
+        while ((buf = xlu_cfg_get_listitem (channels,
+                d_config->num_channels)) != NULL) {
+            libxl_device_channel *chn;
+            libxl_string_list pairs;
+            char *path = NULL;
+            int len;
+
+            chn = ARRAY_EXTEND_INIT(d_config->channels, d_config->num_channels,
+                                   libxl_device_channel_init);
+
+            split_string_into_string_list(buf, ",", &pairs);
+            len = libxl_string_list_length(&pairs);
+            for (i = 0; i < len; i++) {
+                char *key, *key_untrimmed, *value, *value_untrimmed;
+                int rc;
+                rc = split_string_into_pair(pairs[i], "=",
+                                            &key_untrimmed,
+                                            &value_untrimmed);
+                if (rc != 0) {
+                    fprintf(stderr, "failed to parse channel configuration: %s",
+                            pairs[i]);
+                    exit(1);
+                }
+                trim(isspace, key_untrimmed, &key);
+                trim(isspace, value_untrimmed, &value);
+
+                if (!strcmp(key, "backend")) {
+                    replace_string(&chn->backend_domname, value);
+                } else if (!strcmp(key, "name")) {
+                    replace_string(&chn->name, value);
+                } else if (!strcmp(key, "path")) {
+                    replace_string(&path, value);
+                } else if (!strcmp(key, "connection")) {
+                    if (!strcmp(value, "pty")) {
+                        chn->connection = LIBXL_CHANNEL_CONNECTION_PTY;
+                    } else if (!strcmp(value, "socket")) {
+                        chn->connection = LIBXL_CHANNEL_CONNECTION_SOCKET;
+                    } else {
+                        fprintf(stderr, "unknown channel connection '%s'\n",
+                                value);
+                        exit(1);
+                    }
+                } else {
+                    fprintf(stderr, "unknown channel parameter '%s',"
+                                  " ignoring\n", key);
+                }
+                free(key);
+                free(key_untrimmed);
+                free(value);
+                free(value_untrimmed);
+            }
+            switch (chn->connection){
+            case LIBXL_CHANNEL_CONNECTION_UNKNOWN:
+                fprintf(stderr, "channel has unknown 'connection'\n");
+                exit(1);
+            case LIBXL_CHANNEL_CONNECTION_SOCKET:
+                if (!path) {
+                    fprintf(stderr, "channel connection 'socket' requires path=..\n");
+                    exit(1);
+                }
+                chn->u.socket.path = xstrdup(path);
+                break;
+            default:
+                break;
+            }
+            libxl_string_list_dispose(&pairs);
+            free(path);
+        }
+    }
+
     if (!xlu_cfg_get_list (config, "vif", &nics, 0, 0)) {
         d_config->num_nics = 0;
         d_config->nics = NULL;
@@ -1894,13 +2043,6 @@ static int match_option_size(const char *prefix, size_t len,
 #define MATCH_OPTION(prefix, arg, oparg) \
     match_option_size((prefix "="), sizeof((prefix)), (arg), &(oparg))
 
-static void replace_string(char **str, const char *val)
-{
-    free(*str);
-    *str = strdup(val);
-}
-
-
 /* Preserve a copy of a domain under a new name. Updates *r_domid */
 static int preserve_domain(uint32_t *r_domid, libxl_event *event,
                            libxl_domain_config *d_config)
@@ -5932,6 +6074,51 @@ int main_networkdetach(int argc, char **argv)
     return 0;
 }
 
+int main_channellist(int argc, char **argv)
+{
+    int opt;
+    libxl_device_channel *channels;
+    libxl_channelinfo channelinfo;
+    int nb, i;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "channel-list", 1) {
+        /* No options */
+    }
+
+    /*      Idx BE state evt-ch ring-ref connection params*/
+    printf("%-3s %-2s %-5s %-6s %8s %-10s %-30s\n",
+           "Idx", "BE", "state", "evt-ch", "ring-ref", "connection", "");
+    for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
+        uint32_t domid = find_domain(*argv);
+        channels = libxl_device_channel_list(ctx, domid, &nb);
+        if (!channels) {
+            continue;
+        }
+        for (i = 0; i < nb; ++i) {
+            if (!libxl_device_channel_getinfo(ctx, domid, &channels[i],
+                &channelinfo)) {
+                printf("%-3d %-2d ", channels[i].devid, channelinfo.backend_id);
+                printf("%-5d ", channelinfo.state);
+                printf("%-6d %-8d ", channelinfo.evtch, channelinfo.rref);
+                printf("%-10s ", libxl_channel_connection_to_string(
+                       channels[i].connection));
+                switch (channels[i].connection) {
+                    case LIBXL_CHANNEL_CONNECTION_PTY:
+                        printf("%-30s ", channelinfo.u.pty.path);
+                        break;
+                    default:
+                        break;
+                }
+                printf("\n");
+                libxl_channelinfo_dispose(&channelinfo);
+            }
+            libxl_device_channel_dispose(&channels[i]);
+        }
+        free(channels);
+    }
+    return 0;
+}
+
 int main_blockattach(int argc, char **argv)
 {
     int opt;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 7b7fa92..da8435e 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -335,6 +335,11 @@ struct cmd_spec cmd_table[] = {
       "Destroy a domain's virtual network device",
       "<Domain> <DevId|mac>",
     },
+    { "channel-list",
+      &main_channellist, 0, 0,
+      "List virtual channel devices for a domain",
+      "<Domain(s)>",
+    },
     { "block-attach",
       &main_blockattach, 1, 1,
       "Create a new virtual block device",
-- 
1.7.10.4

Re: [PATCH v5 for-4.5 1/2] libxl: add support for 'channels'

[Ian Jackson]

David Scott writes ("[PATCH v5 for-4.5 1/2] libxl: add support for 'channels'"):
> 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.

This looks good in principle.

I have a couple of easy quibbles:

You have forgotten to document that you transport the channel name in
xenstore in the subkey `name'.  This should be in your `console.txt'
patch I think, given that xenstore-paths.markdown refers to that for
all the nodes in .../console.

And the interaction between consoles in the console part of the domain
configuration and the channels listed in the channels part, should be
documented.  (Even if it's just that consoles always have no name and
channels always have one.)


I have a harder one:

> +int libxl__init_console_from_channel(libxl__gc *gc,
> +                                     libxl__device_console *console,
> +                                     int dev_num,
> +                                     libxl_device_channel *channel)

AFAICT this function is used to recreate the domain's channel
configuration info for the benefit of libxl's caller (making
enquiries) and setting up the qemu arguments.

But nowadays, following Wei's save/restore/JSON patches, I think we
would expect libxl to retrieve this information from the JSON
configuration.  That is, the console information should be in the
stored JSON config and can be retrieved from there.

(There are also unfortunate security implications to reading the
backend directory like that - if we have a driver domain, the qemu
might get untrustworthy paths.)

Wei, am I right ?


Thanks,
Ian.

Re: [PATCH v5 for-4.5 2/2] xl: add support for 'channels'

[Ian Jackson]

David Scott writes ("[PATCH v5 for-4.5 2/2] xl: add support for 'channels'"):
> This adds support for channel declarations of the form:

This looks pretty good to me.  I have some small complaints:

> +Each B<CHANNEL_SPEC_STRING> is a comma-separated list of C<KEY=VALUE>
> +settings, from the following list:

You should specify that spaces are stripped, and where.

You should also probably mention that there is no way to include a
comma, or AFAICT leading/trailing whitespace, in any of the values of
these fields.

> +static char *xstrdup(const char *x)
> +{
> +    char *r;
> +    r = strdup(x);

Breaking this kind of thing out into a separate patch would make
review a bit easier.  This applies the movement of replace_string and
arguably to the introduction of the trim and split functions.

> +typedef int (*char_predicate_t)(const int c);

You need a comment here about the bizarre interface to ctype.h macros.
See the comment by the CTYPE macro in libxl_internal.h.

> +static void trim(char_predicate_t predicate, const char *input, char **output)
...
> +    while ((*p != '\000') && (predicate(*p)))

And these calls to predicate() are wrong because they don't take
account of the ctype API bug.  (Please don't ask why they left this
ridiculous landmine here...)

I haven't reviewed your string fiddling in detail.  It would probaby
be good for someone to do so because this kind of thing can contain
security bugs (which are relevant if anyone constructs an xl domain
config out of laundered pieces some of which are supplied by an
attacker).

> +int main_channellist(int argc, char **argv)
> +{

What is the way to get the channel list in machine-readable JSON
format ?

Thanks,
Ian.

Re: [PATCH v5 for-4.5 1/2] libxl: add support for 'channels'

[Wei Liu]

On Mon, Sep 22, 2014 at 05:17:20PM +0100, Ian Jackson wrote:
> David Scott writes ("[PATCH v5 for-4.5 1/2] libxl: add support for 'channels'"):
> > 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.
> 
> This looks good in principle.
> 
> I have a couple of easy quibbles:
> 
> You have forgotten to document that you transport the channel name in
> xenstore in the subkey `name'.  This should be in your `console.txt'
> patch I think, given that xenstore-paths.markdown refers to that for
> all the nodes in .../console.
> 
> And the interaction between consoles in the console part of the domain
> configuration and the channels listed in the channels part, should be
> documented.  (Even if it's just that consoles always have no name and
> channels always have one.)
> 
> 
> I have a harder one:
> 
> > +int libxl__init_console_from_channel(libxl__gc *gc,
> > +                                     libxl__device_console *console,
> > +                                     int dev_num,
> > +                                     libxl_device_channel *channel)
> 
> AFAICT this function is used to recreate the domain's channel
> configuration info for the benefit of libxl's caller (making
> enquiries) and setting up the qemu arguments.
> 
> But nowadays, following Wei's save/restore/JSON patches, I think we
> would expect libxl to retrieve this information from the JSON
> configuration.  That is, the console information should be in the
> stored JSON config and can be retrieved from there.
> 

The design Dave proposed has dedicated libxl_device_channel structure,
and that's the thing being saved in JSON.

AIUI this channel device happens to be backed by a QEMU console device,
which doesn't precludes it from being backed by dedicated backend or
other devices. In this case I think generating a console device
internally in libxl is actually the right thing to do.

Dave, am I right about the design?

Wei.

> (There are also unfortunate security implications to reading the
> backend directory like that - if we have a driver domain, the qemu
> might get untrustworthy paths.)
> 
> Wei, am I right ?
> 
> 
> Thanks,
> Ian.

Re: [PATCH v5 for-4.5 1/2] libxl: add support for 'channels'

[Dave Scott]

On 23 Sep 2014, at 11:16, Wei Liu <wei.liu2@citrix.com> wrote:

> On Mon, Sep 22, 2014 at 05:17:20PM +0100, Ian Jackson wrote:
>> David Scott writes ("[PATCH v5 for-4.5 1/2] libxl: add support for 'channels'"):
>>> 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.
>> 
>> This looks good in principle.
>> 
>> I have a couple of easy quibbles:
>> 
>> You have forgotten to document that you transport the channel name in
>> xenstore in the subkey `name'.  This should be in your `console.txt'
>> patch I think, given that xenstore-paths.markdown refers to that for
>> all the nodes in .../console.
>> 
>> And the interaction between consoles in the console part of the domain
>> configuration and the channels listed in the channels part, should be
>> documented.  (Even if it's just that consoles always have no name and
>> channels always have one.)
>> 
>> 
>> I have a harder one:
>> 
>>> +int libxl__init_console_from_channel(libxl__gc *gc,
>>> +                                     libxl__device_console *console,
>>> +                                     int dev_num,
>>> +                                     libxl_device_channel *channel)
>> 
>> AFAICT this function is used to recreate the domain's channel
>> configuration info for the benefit of libxl's caller (making
>> enquiries) and setting up the qemu arguments.
>> 
>> But nowadays, following Wei's save/restore/JSON patches, I think we
>> would expect libxl to retrieve this information from the JSON
>> configuration.  That is, the console information should be in the
>> stored JSON config and can be retrieved from there.
>> 
> 
> The design Dave proposed has dedicated libxl_device_channel structure,
> and that's the thing being saved in JSON.
> 
> AIUI this channel device happens to be backed by a QEMU console device,
> which doesn't precludes it from being backed by dedicated backend or
> other devices. In this case I think generating a console device
> internally in libxl is actually the right thing to do.
> 
> Dave, am I right about the design?

That’s right. I’ve added a libxl_device_channel to the domain config, which should be being saved in the JSON (is that automatic? I’ve not looked at the code for it yet.) The libxl__device_console remains an internal type. The existing domain create codepath synthesises an instance of libxl__device_console to represent the primary console. My patches extend this to create secondary consoles to implement the channels.

So if a domain is saved then the libxl_device_channels should end up in the JSON. On restore, these libxl_device_channels should get reloaded from the JSON and internally mapped onto libxl__device_consoles — does that sound right?

Thanks,
Dave
> 
> Wei.
> 
>> (There are also unfortunate security implications to reading the
>> backend directory like that - if we have a driver domain, the qemu
>> might get untrustworthy paths.)
>> 
>> Wei, am I right ?
>> 
>> 
>> Thanks,
>> Ian.

Re: [PATCH v5 for-4.5 1/2] libxl: add support for 'channels'

[Wei Liu]

On Tue, Sep 23, 2014 at 11:35:19AM +0100, Dave Scott wrote:
> 
> On 23 Sep 2014, at 11:16, Wei Liu <wei.liu2@citrix.com> wrote:
> 
> > On Mon, Sep 22, 2014 at 05:17:20PM +0100, Ian Jackson wrote:
> >> David Scott writes ("[PATCH v5 for-4.5 1/2] libxl: add support for 'channels'"):
> >>> 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.
> >> 
> >> This looks good in principle.
> >> 
> >> I have a couple of easy quibbles:
> >> 
> >> You have forgotten to document that you transport the channel name in
> >> xenstore in the subkey `name'.  This should be in your `console.txt'
> >> patch I think, given that xenstore-paths.markdown refers to that for
> >> all the nodes in .../console.
> >> 
> >> And the interaction between consoles in the console part of the domain
> >> configuration and the channels listed in the channels part, should be
> >> documented.  (Even if it's just that consoles always have no name and
> >> channels always have one.)
> >> 
> >> 
> >> I have a harder one:
> >> 
> >>> +int libxl__init_console_from_channel(libxl__gc *gc,
> >>> +                                     libxl__device_console *console,
> >>> +                                     int dev_num,
> >>> +                                     libxl_device_channel *channel)
> >> 
> >> AFAICT this function is used to recreate the domain's channel
> >> configuration info for the benefit of libxl's caller (making
> >> enquiries) and setting up the qemu arguments.
> >> 
> >> But nowadays, following Wei's save/restore/JSON patches, I think we
> >> would expect libxl to retrieve this information from the JSON
> >> configuration.  That is, the console information should be in the
> >> stored JSON config and can be retrieved from there.
> >> 
> > 
> > The design Dave proposed has dedicated libxl_device_channel structure,
> > and that's the thing being saved in JSON.
> > 
> > AIUI this channel device happens to be backed by a QEMU console device,
> > which doesn't precludes it from being backed by dedicated backend or
> > other devices. In this case I think generating a console device
> > internally in libxl is actually the right thing to do.
> > 
> > Dave, am I right about the design?
> 
> That’s right. I’ve added a libxl_device_channel to the domain config,
> which should be being saved in the JSON (is that automatic? I’ve not
> looked at the code for it yet.) The libxl__device_console remains an

Yes, it will be saved in JSON provided xl feeds it to libxl in the first
place.

But if you plan to add in channel hotplug / remove functionality, you
need to look at how other devices do that.

> internal type. The existing domain create codepath synthesises an
> instance of libxl__device_console to represent the primary console. My
> patches extend this to create secondary consoles to implement the
> channels.
> 
> So if a domain is saved then the libxl_device_channels should end up
> in the JSON. On restore, these libxl_device_channels should get
> reloaded from the JSON and internally mapped onto
> libxl__device_consoles — does that sound right?
> 

That sounds right to me.

Wei.

> Thanks,
> Dave
> > 
> > Wei.
> > 
> >> (There are also unfortunate security implications to reading the
> >> backend directory like that - if we have a driver domain, the qemu
> >> might get untrustworthy paths.)
> >> 
> >> Wei, am I right ?
> >> 
> >> 
> >> Thanks,
> >> Ian.

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

Re: [PATCH v5 for-4.5 2/2] xl: add support for 'channels'

[Dave Scott]

On 22 Sep 2014, at 17:25, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:

> David Scott writes ("[PATCH v5 for-4.5 2/2] xl: add support for 'channels'"):
>> This adds support for channel declarations of the form:
> 
> This looks pretty good to me.  I have some small complaints:
> 
>> +Each B<CHANNEL_SPEC_STRING> is a comma-separated list of C<KEY=VALUE>
>> +settings, from the following list:
> 
> You should specify that spaces are stripped, and where.
> 
> You should also probably mention that there is no way to include a
> comma, or AFAICT leading/trailing whitespace, in any of the values of
> these fields.

Good point.

> 
>> +static char *xstrdup(const char *x)
>> +{
>> +    char *r;
>> +    r = strdup(x);
> 
> Breaking this kind of thing out into a separate patch would make
> review a bit easier.  This applies the movement of replace_string and
> arguably to the introduction of the trim and split functions.

OK. I’ll split these out for review and make it easy to squash them back
together (if that’s desired).

> 
>> +typedef int (*char_predicate_t)(const int c);
> 
> You need a comment here about the bizarre interface to ctype.h macros.
> See the comment by the CTYPE macro in libxl_internal.h.
> 
>> +static void trim(char_predicate_t predicate, const char *input, char **output)
> ...
>> +    while ((*p != '\000') && (predicate(*p)))
> 
> And these calls to predicate() are wrong because they don't take
> account of the ctype API bug.  (Please don't ask why they left this
> ridiculous landmine here…)

Thanks, I’d never noticed that before!

> 
> I haven't reviewed your string fiddling in detail.  It would probaby
> be good for someone to do so because this kind of thing can contain
> security bugs (which are relevant if anyone constructs an xl domain
> config out of laundered pieces some of which are supplied by an
> attacker).
> 
>> +int main_channellist(int argc, char **argv)
>> +{
> 
> What is the way to get the channel list in machine-readable JSON
> format ?

‘xl list —long’ (via list_domains_details) can display the json:

$ sudo xl list --long
[
    {
        "domid": 30,
...
            "channels": [
                {
                    "devid": 0,
                    "name": "foo",
                    "connection.socket": {
                        "path": "/var/lib/libvirt/qemu/s-4-VM.agent"
                    }
                }
            ],

...

There’s no option to ‘xl channel-list’ to show them in JSON. I notice
the xl hotplug commands have a ‘dryrun’ option which dumps the JSON of
the device they would have added, but I’ve not implemented channel
hotplug (yet)

Cheers,
Dave

Re: [PATCH v5 for-4.5 1/2] libxl: add support for 'channels'

[Dave Scott]

On 22 Sep 2014, at 17:17, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:

> David Scott writes ("[PATCH v5 for-4.5 1/2] libxl: add support for 'channels'"):
>> 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.
> 
> This looks good in principle.
> 
> I have a couple of easy quibbles:
> 
> You have forgotten to document that you transport the channel name in
> xenstore in the subkey `name'.  This should be in your `console.txt'
> patch I think, given that xenstore-paths.markdown refers to that for
> all the nodes in .../console.

Good spot. I think I’m blind to some of these documentation problems.

> And the interaction between consoles in the console part of the domain
> configuration and the channels listed in the channels part, should be
> documented.  (Even if it's just that consoles always have no name and
> channels always have one.)

I’ve added some comments to the domain configuration definition in the
IDL and to libxl.h. Interestingly the console device is still an
internal type in libxl_types_internal.idl and not user-configurable via
the libxl_domain_config, so that removes one source of confusion (hopefully)

Cheers,
Dave

Re: [PATCH v5 for-4.5 1/2] libxl: add support for 'channels'

[Dave Scott]

On 22 Sep 2014, at 17:17, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:

> David Scott writes ("[PATCH v5 for-4.5 1/2] libxl: add support for 'channels'"):
>> 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.
> 
> This looks good in principle.
> 
> I have a couple of easy quibbles:
> 
> You have forgotten to document that you transport the channel name in
> xenstore in the subkey `name'.  This should be in your `console.txt'
> patch I think, given that xenstore-paths.markdown refers to that for
> all the nodes in .../console.
> 
> And the interaction between consoles in the console part of the domain
> configuration and the channels listed in the channels part, should be
> documented.  (Even if it's just that consoles always have no name and
> channels always have one.)
> 
> 
> I have a harder one:
> 
>> +int libxl__init_console_from_channel(libxl__gc *gc,
>> +                                     libxl__device_console *console,
>> +                                     int dev_num,
>> +                                     libxl_device_channel *channel)
> 
> AFAICT this function is used to recreate the domain's channel
> configuration info for the benefit of libxl's caller (making
> enquiries) and setting up the qemu arguments.
> 
> But nowadays, following Wei's save/restore/JSON patches, I think we
> would expect libxl to retrieve this information from the JSON
> configuration.  That is, the console information should be in the
> stored JSON config and can be retrieved from there.
> 
> (There are also unfortunate security implications to reading the
> backend directory like that - if we have a driver domain, the qemu
> might get untrustworthy paths.)

Sorry I missed this bit, thanks Wei for highlighting it.

I agree that we must be careful not to start a driver domain and tell it
‘talk to the socket at path x’ when in fact ‘x’ is in a different domain.
At the moment paths are in the driver domain and the person who writes
the domain config must consider this when they choose their driver domain.

Thinking about the future, although we could arrange it such that the
data gets routed to other places (e.g. run qemu/xenconsoled in the driver
domain, have it proxy the data over a vchan, have a thing in dom0 proxy
to the right socket) I suspect that we’ll want to add additional
‘connection’ behaviours which reference resources via global names rather
than local ones. I quite like the idea of adding some kind of URI to
name a network endpoint and have the driver domain proxy all the data
to/from there.

I’m not totally sure that’s what you were getting at — sorry if I’ve missed
the point!

Cheers,
Dave

> Wei, am I right ?
> 
> 
> Thanks,
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel