xl, libxl: add support for 'channels'

xl, libxl: add support for 'channels'

[David Scott]

Hi,

I've split out the xl string fiddling into a couple of patches to make
them easier to review. I've added __attribute__((unused)) in these commits
to keep it building and taken them away again in the last patch.

Most of the rest of the changes are to docs, and comments in idl/headers.
I think I'm too familiar with the low-level details to see where the docs
are obviously missing :-)
 
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 v5:
* Fix use of <ctype.h> functions, and add a warning note
* Split out string fiddling into separate squash-able patches.
  (Note I've added __attribute__((unused)) to keep it building)
* doc: clearly describe leading/trailing whitespace handling and that
  [,="] are banned characters
* doc: mention the xenstore key 'name' (oops)
* doc: mention in the IDL and libxl.h that channels manifest as consoles
  with names. Note that consoles are still considered an internal type
  (in libxl_types_internal.idl) and don't appear in libxl_domain_config.
  Channels are user-configurable and do appear in libxl_domain_config.

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 v6 for-4.5 1/5] 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                |   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.
+  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;
+    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;
@@ -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);
     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);
@@ -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

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

[Konrad Rzeszutek Wilk]

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

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

[Wei Liu]

In general this looks good to me.

Ian Jackson asked one question which remained unanswered in the last
iteration.

QUOTE
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.
ENDQUOTE

Has that question become moot? Did I miss anything?

Wei.

[PATCH v6 for-4.5 2/5] xl: move 'replace_string' further up the file

[David Scott]

This allows the function to be reused more easily.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 698b3bc..1695f74 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -799,6 +799,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,
@@ -1914,13 +1920,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)
-- 
1.7.10.4

Re: [PATCH v6 for-4.5 2/5] xl: move 'replace_string' further up the file

[Konrad Rzeszutek Wilk]

On Wed, Sep 24, 2014 at 09:48:02PM +0100, David Scott wrote:
> This allows the function to be reused more easily.

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Signed-off-by: David Scott <dave.scott@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 698b3bc..1695f74 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -799,6 +799,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,
> @@ -1914,13 +1920,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)
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v6 for-4.5 2/5] xl: move 'replace_string' further up the file

[Wei Liu]

On Wed, Sep 24, 2014 at 09:48:02PM +0100, David Scott wrote:
> This allows the function to be reused more easily.
> 
> Signed-off-by: David Scott <dave.scott@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 698b3bc..1695f74 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -799,6 +799,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);
> +}
> +

If you rearrange patch 2 and 3 you can then use xstrdup here.

Wei.

[PATCH v6 for-4.5 3/5] xl: add 'xstrdup' next to 'xrealloc'

[David Scott]

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1695f74..1fc2171 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -300,6 +300,19 @@ static void *xrealloc(void *ptr, size_t sz) {
     return r;
 }
 
+static char *xstrdup(const char *x) __attribute__ ((unused));
+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);               \
-- 
1.7.10.4

Re: [PATCH v6 for-4.5 3/5] xl: add 'xstrdup' next to 'xrealloc'

[Konrad Rzeszutek Wilk]

On Wed, Sep 24, 2014 at 09:48:03PM +0100, David Scott wrote:
> Signed-off-by: David Scott <dave.scott@citrix.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1695f74..1fc2171 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -300,6 +300,19 @@ static void *xrealloc(void *ptr, size_t sz) {
>      return r;
>  }
>  
> +static char *xstrdup(const char *x) __attribute__ ((unused));
> +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);               \
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v6 for-4.5 3/5] xl: add 'xstrdup' next to 'xrealloc'

[Wei Liu]

On Wed, Sep 24, 2014 at 09:48:03PM +0100, David Scott wrote:
> Signed-off-by: David Scott <dave.scott@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxl/xl_cmdimpl.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1695f74..1fc2171 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -300,6 +300,19 @@ static void *xrealloc(void *ptr, size_t sz) {
>      return r;
>  }
>  
> +static char *xstrdup(const char *x) __attribute__ ((unused));
> +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);               \
> -- 
> 1.7.10.4

[PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions

[David Scott]

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   66 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 1fc2171..d6f311f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -300,7 +300,6 @@ static void *xrealloc(void *ptr, size_t sz) {
     return r;
 }
 
-static char *xstrdup(const char *x) __attribute__ ((unused));
 static char *xstrdup(const char *x)
 {
     char *r;
@@ -564,6 +563,71 @@ static void split_string_into_string_list(const char *str,
     free(s);
 }
 
+/* NB: this follows the interface used by <ctype.h>. See 'man 3 ctype'
+   and look for CTYPE in libxl_internal.h */
+typedef int (*char_predicate_t)(const int c);
+
+static void trim(char_predicate_t predicate, const char *input, char **output) __attribute__ ((unused));
+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((unsigned char)*p)))
+        p ++;
+    q = p + strlen(p) - 1;
+    /* q points to the last non-NULL character */
+    while ((q > p) && (predicate((unsigned char)*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) __attribute__ ((unused));
+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;
-- 
1.7.10.4

Re: [PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions

[Konrad Rzeszutek Wilk]

On Wed, Sep 24, 2014 at 09:48:04PM +0100, David Scott wrote:
> Signed-off-by: David Scott <dave.scott@citrix.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> ---
>  tools/libxl/xl_cmdimpl.c |   66 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1fc2171..d6f311f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -300,7 +300,6 @@ static void *xrealloc(void *ptr, size_t sz) {
>      return r;
>  }
>  
> -static char *xstrdup(const char *x) __attribute__ ((unused));
>  static char *xstrdup(const char *x)
>  {
>      char *r;
> @@ -564,6 +563,71 @@ static void split_string_into_string_list(const char *str,
>      free(s);
>  }
>  
> +/* NB: this follows the interface used by <ctype.h>. See 'man 3 ctype'
> +   and look for CTYPE in libxl_internal.h */
> +typedef int (*char_predicate_t)(const int c);
> +
> +static void trim(char_predicate_t predicate, const char *input, char **output) __attribute__ ((unused));
> +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((unsigned char)*p)))
> +        p ++;
> +    q = p + strlen(p) - 1;
> +    /* q points to the last non-NULL character */
> +    while ((q > p) && (predicate((unsigned char)*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) __attribute__ ((unused));
> +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;
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions

[Wei Liu]

On Wed, Sep 24, 2014 at 09:48:04PM +0100, David Scott wrote:
> Signed-off-by: David Scott <dave.scott@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   66 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1fc2171..d6f311f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -300,7 +300,6 @@ static void *xrealloc(void *ptr, size_t sz) {
>      return r;
>  }
>  
> -static char *xstrdup(const char *x) __attribute__ ((unused));
>  static char *xstrdup(const char *x)
>  {
>      char *r;
> @@ -564,6 +563,71 @@ static void split_string_into_string_list(const char *str,
>      free(s);
>  }
>  
> +/* NB: this follows the interface used by <ctype.h>. See 'man 3 ctype'
> +   and look for CTYPE in libxl_internal.h */
> +typedef int (*char_predicate_t)(const int c);
> +
> +static void trim(char_predicate_t predicate, const char *input, char **output) __attribute__ ((unused));
> +static void trim(char_predicate_t predicate, const char *input, char **output)
> +{
> +    char *p, *q, *tmp;

Ideally you should check input != NULL before dereferencing it. Or you
need to document this function expects a valid pointer.

> +    if (*input == '\000')
> +        return;

As this function won't fail, caller cannot distinguish between a valid
pointer and garbage. I think you need to set output to NULL in this
case.

> +    /* Input has length >= 1 */
> +
> +    p = tmp = xstrdup(input);
> +    /* Skip past the first whitespace */

This comment is wrong.

> +    while ((*p != '\000') && (predicate((unsigned char)*p)))
> +        p ++;
> +    q = p + strlen(p) - 1;
> +    /* q points to the last non-NULL character */
> +    while ((q > p) && (predicate((unsigned char)*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) __attribute__ ((unused));
> +static int split_string_into_pair(const char *str,
> +                                  const char *delim,
> +                                  char **a,
> +                                  char **b)
> +{

You didn't restrict number of entries in this function. So a malformed
"a=b=c" string ends up with "a" in key and "b=c" in value. Is that a
problem?

If value is to be passed directly to any other component (QEMU?) that
supports similar syntax as argument then I think we should be care about
this string.

Wei.

Re: [PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions"):
> On Wed, Sep 24, 2014 at 09:48:04PM +0100, David Scott wrote:
> > +/* NB: this follows the interface used by <ctype.h>. See 'man 3 ctype'
> > +   and look for CTYPE in libxl_internal.h */
> > +typedef int (*char_predicate_t)(const int c);
> > +
> > +static void trim(char_predicate_t predicate, const char *input, char **output) __attribute__ ((unused));
> > +static void trim(char_predicate_t predicate, const char *input, char **output)
> > +{
> > +    char *p, *q, *tmp;
> 
> Ideally you should check input != NULL before dereferencing it. Or you
> need to document this function expects a valid pointer.

(Thanks for looking at this, Wei, but:)

I disagree.  I think in general a function that takes any kind of
pointer should be assumed to require a non-NULL pointer.  It should
not carry out any kind of nullness check; it should just dereference
the pointer (and consequently crash if it is NULL).

If a function permits callers to pass NULL, that should be documented
in the function's doc comment (along, presumably, with the semantics,
if that isn't obvious).

> You didn't restrict number of entries in this function. So a malformed
> "a=b=c" string ends up with "a" in key and "b=c" in value. Is that a
> problem?

That seems desirable to me.  Otherwise `=' becomes another character
that can't be specified in a value.

Ian.

Re: [PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions

[Wei Liu]

On Fri, Sep 26, 2014 at 04:45:30PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions"):
> > On Wed, Sep 24, 2014 at 09:48:04PM +0100, David Scott wrote:
> > > +/* NB: this follows the interface used by <ctype.h>. See 'man 3 ctype'
> > > +   and look for CTYPE in libxl_internal.h */
> > > +typedef int (*char_predicate_t)(const int c);
> > > +
> > > +static void trim(char_predicate_t predicate, const char *input, char **output) __attribute__ ((unused));
> > > +static void trim(char_predicate_t predicate, const char *input, char **output)
> > > +{
> > > +    char *p, *q, *tmp;
> > 
> > Ideally you should check input != NULL before dereferencing it. Or you
> > need to document this function expects a valid pointer.
> 
> (Thanks for looking at this, Wei, but:)
> 
> I disagree.  I think in general a function that takes any kind of
> pointer should be assumed to require a non-NULL pointer.  It should
> not carry out any kind of nullness check; it should just dereference
> the pointer (and consequently crash if it is NULL).
> 
> If a function permits callers to pass NULL, that should be documented
> in the function's doc comment (along, presumably, with the semantics,
> if that isn't obvious).
> 

Fair enough. I don't really have strong preference on this kind of
issue. We just need to have an agreement on which route we take. 

Wei.

Re: [PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v6 for-4.5 4/5] xl: add 'trim' and 'split_string_into_pair' functions"):
> On Fri, Sep 26, 2014 at 04:45:30PM +0100, Ian Jackson wrote:
> > I disagree.  I think in general a function that takes any kind of
> > pointer should be assumed to require a non-NULL pointer.  It should
> > not carry out any kind of nullness check; it should just dereference
> > the pointer (and consequently crash if it is NULL).
...
> Fair enough. I don't really have strong preference on this kind of
> issue. We just need to have an agreement on which route we take. 

Right.  The rest of libxl follows the convention I describe above.  So
I think this aspect of Dave's patch is fine.

Iabn.

[PATCH v6 for-4.5 5/5] 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     |   53 +++++++++++++++++++
 docs/man/xl.pod.1         |   10 ++++
 tools/libxl/xl.h          |    1 +
 tools/libxl/xl_cmdimpl.c  |  129 ++++++++++++++++++++++++++++++++++++++++++---
 tools/libxl/xl_cmdtable.c |    5 ++
 5 files changed, 190 insertions(+), 8 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 517ae2f..6626323 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -530,6 +530,59 @@ 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>
+seettings. Leading and trailing whitespace is ignored in both KEY and
+VALUE. Neither KEY nor VALUE may contain ',', '=' or '"'. Defined values
+are:
+
+=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 f1e95db..35aa3a4 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1215,6 +1215,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 d6f311f..89a8697 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -530,7 +530,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);
     }
 
@@ -546,7 +546,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);
     }
 
@@ -567,7 +567,6 @@ static void split_string_into_string_list(const char *str,
    and look for CTYPE in libxl_internal.h */
 typedef int (*char_predicate_t)(const int c);
 
-static void trim(char_predicate_t predicate, const char *input, char **output) __attribute__ ((unused));
 static void trim(char_predicate_t predicate, const char *input, char **output)
 {
     char *p, *q, *tmp;
@@ -593,10 +592,6 @@ static void trim(char_predicate_t predicate, const char *input, char **output)
 static int split_string_into_pair(const char *str,
                                   const char *delim,
                                   char **a,
-                                  char **b) __attribute__ ((unused));
-static int split_string_into_pair(const char *str,
-                                  const char *delim,
-                                  char **a,
                                   char **b)
 {
     char *s, *p, *saveptr, *aa = NULL, *bb = NULL;
@@ -891,7 +886,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;
@@ -1376,6 +1371,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;
@@ -6045,6 +6113,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 35f02c4..a6df234 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -337,6 +337,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 v6 for-4.5 5/5] xl: add support for 'channels'

[Konrad Rzeszutek Wilk]

On Wed, Sep 24, 2014 at 09:48:05PM +0100, David Scott wrote:
> 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     |   53 +++++++++++++++++++
>  docs/man/xl.pod.1         |   10 ++++
>  tools/libxl/xl.h          |    1 +
>  tools/libxl/xl_cmdimpl.c  |  129 ++++++++++++++++++++++++++++++++++++++++++---
>  tools/libxl/xl_cmdtable.c |    5 ++
>  5 files changed, 190 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 517ae2f..6626323 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -530,6 +530,59 @@ 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>
> +seettings. Leading and trailing whitespace is ignored in both KEY and
> +VALUE. Neither KEY nor VALUE may contain ',', '=' or '"'. Defined values
> +are:
> +
> +=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 f1e95db..35aa3a4 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1215,6 +1215,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 d6f311f..89a8697 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -530,7 +530,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);
>      }
>  
> @@ -546,7 +546,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);
>      }
>  
> @@ -567,7 +567,6 @@ static void split_string_into_string_list(const char *str,
>     and look for CTYPE in libxl_internal.h */
>  typedef int (*char_predicate_t)(const int c);
>  
> -static void trim(char_predicate_t predicate, const char *input, char **output) __attribute__ ((unused));
>  static void trim(char_predicate_t predicate, const char *input, char **output)
>  {
>      char *p, *q, *tmp;
> @@ -593,10 +592,6 @@ static void trim(char_predicate_t predicate, const char *input, char **output)
>  static int split_string_into_pair(const char *str,
>                                    const char *delim,
>                                    char **a,
> -                                  char **b) __attribute__ ((unused));
> -static int split_string_into_pair(const char *str,
> -                                  const char *delim,
> -                                  char **a,
>                                    char **b)
>  {
>      char *s, *p, *saveptr, *aa = NULL, *bb = NULL;
> @@ -891,7 +886,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;
> @@ -1376,6 +1371,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){

Your editor ate a space.

> +            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;
> @@ -6045,6 +6113,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;
> +        }

I think you can drop the '{}'.

> +        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 35f02c4..a6df234 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -337,6 +337,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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Wei Liu]

On Wed, Sep 24, 2014 at 09:48:05PM +0100, David Scott wrote:
[...]
> +=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>:
> +

I don't think you need that ":"

> +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>:
> +

Ditto.

> +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 f1e95db..35aa3a4 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1215,6 +1215,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 d6f311f..89a8697 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -530,7 +530,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);
>      }
>  
> @@ -546,7 +546,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);
>      }
>  
> @@ -567,7 +567,6 @@ static void split_string_into_string_list(const char *str,
>     and look for CTYPE in libxl_internal.h */
>  typedef int (*char_predicate_t)(const int c);
>  
> -static void trim(char_predicate_t predicate, const char *input, char **output) __attribute__ ((unused));
>  static void trim(char_predicate_t predicate, const char *input, char **output)
>  {
>      char *p, *q, *tmp;
> @@ -593,10 +592,6 @@ static void trim(char_predicate_t predicate, const char *input, char **output)
>  static int split_string_into_pair(const char *str,
>                                    const char *delim,
>                                    char **a,
> -                                  char **b) __attribute__ ((unused));
[...]
> +            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;

Don't need to do anything for 'PTY' connection? If so, please as a
comment here.

> +            default:
> +                break;

So this branch covers 'PTY' case? If so, please document, if not it
should have exit(1).

I can see from the code at this point chn->connection is limited to one
of the three types defined so far, but I would rather be explicit. That
is, have 

   case LIBXL_CHANNEL_CONNECTION_PTY:
       /* nothing to do */
       break;
   default:
       fprintf("something's wrong\n");
       exit(1);

Wei.

Re: xl, libxl: add support for 'channels'

[Konrad Rzeszutek Wilk]

On Wed, Sep 24, 2014 at 09:48:00PM +0100, David Scott wrote:
> Hi,
> 
> I've split out the xl string fiddling into a couple of patches to make
> them easier to review. I've added __attribute__((unused)) in these commits
> to keep it building and taken them away again in the last patch.
> 
> Most of the rest of the changes are to docs, and comments in idl/headers.
> I think I'm too familiar with the low-level details to see where the docs
> are obviously missing :-)

I looked at the patches and I had mostly syntax questions and maybe
spotted two memory leaks.

The major piece of information that is missing is - who/what generates
the udev information in the guest? I could not find it in the Linux
hvc_console.c driver so I am unclear of how it is suppose to
work there?

Also, while my Reviewed-by is nice, either of the three maintainers:
 Wei, IanJ, and IanC would need to Review these patches as well.

>  
> 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 v5:
> * Fix use of <ctype.h> functions, and add a warning note
> * Split out string fiddling into separate squash-able patches.
>   (Note I've added __attribute__((unused)) to keep it building)
> * doc: clearly describe leading/trailing whitespace handling and that
>   [,="] are banned characters
> * doc: mention the xenstore key 'name' (oops)
> * doc: mention in the IDL and libxl.h that channels manifest as consoles
>   with names. Note that consoles are still considered an internal type
>   (in libxl_types_internal.idl) and don't appear in libxl_domain_config.
>   Channels are user-configurable and do appear in libxl_domain_config.
> 
> 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>
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: xl, libxl: add support for 'channels'

[Dave Scott]

Hi Konrad,

On 25 Sep 2014, at 20:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Wed, Sep 24, 2014 at 09:48:00PM +0100, David Scott wrote:
>> Hi,
>> 
>> I've split out the xl string fiddling into a couple of patches to make
>> them easier to review. I've added __attribute__((unused)) in these commits
>> to keep it building and taken them away again in the last patch.
>> 
>> Most of the rest of the changes are to docs, and comments in idl/headers.
>> I think I'm too familiar with the low-level details to see where the docs
>> are obviously missing :-)
> 
> I looked at the patches and I had mostly syntax questions and maybe
> spotted two memory leaks.

Thanks for those, I’ll fix up the problems.

> The major piece of information that is missing is - who/what generates
> the udev information in the guest? I could not find it in the Linux
> hvc_console.c driver so I am unclear of how it is suppose to
> work there?

I believe all secondary consoles (in device/console/ in xenstore) generate the
event. I’m not familiar with the kernel side of things; Stefano: can you point
us in the right direction?

For reference I used a udev rule to catch all secondary consoles:

# Set up secondary Xen consoles
SUBSYSTEM=="xen", DEVPATH=="/devices/console-[0-9]", RUN+="xenconsole-setup-tty”

and then I wrote a simple shell script to read the optional ‘name’ key from
xenstore and, if present, create a device node with a well-known name rather
than the default one.

For reference my hacky test script is here: 

https://github.com/djs55/mirage-console/blob/master/udev/xenconsole-setup-tty

For an end-to-end test I took cloudstack and added this udev rule to its
helper VM. I tweaked the helper VM software to open my named device node
rather than the KVM /dev/vport one. IIRC I also had to also switch the tty
into raw mode. After that it worked: the cloudstack agent connected to the
dom0 socket and was able to exchange data with the domU software.

This was all inspired by the Daniel B from libvirt describing the libvirt
channels as “consoles with names”. I took him literally :-)

> Also, while my Reviewed-by is nice, either of the three maintainers:
> Wei, IanJ, and IanC would need to Review these patches as well.

Indeed.

Cheers,
Dave

> 
>> 
>> 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 v5:
>> * Fix use of <ctype.h> functions, and add a warning note
>> * Split out string fiddling into separate squash-able patches.
>>  (Note I've added __attribute__((unused)) to keep it building)
>> * doc: clearly describe leading/trailing whitespace handling and that
>>  [,="] are banned characters
>> * doc: mention the xenstore key 'name' (oops)
>> * doc: mention in the IDL and libxl.h that channels manifest as consoles
>>  with names. Note that consoles are still considered an internal type
>>  (in libxl_types_internal.idl) and don't appear in libxl_domain_config.
>>  Channels are user-configurable and do appear in libxl_domain_config.
>> 
>> 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>
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: xl, libxl: add support for 'channels'

[Ian Jackson]

Dave Scott writes ("Re: [Xen-devel] xl, libxl: add support for 'channels'"):
> On 25 Sep 2014, at 20:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > The major piece of information that is missing is - who/what generates
> > the udev information in the guest? I could not find it in the Linux
> > hvc_console.c driver so I am unclear of how it is suppose to
> > work there?
> 
> I believe all secondary consoles (in device/console/ in xenstore)
> generate the event. I’m not familiar with the kernel side of things;
> Stefano: can you point us in the right direction?
> 
> For reference I used a udev rule to catch all secondary consoles:
> 
> # Set up secondary Xen consoles
> SUBSYSTEM=="xen", DEVPATH=="/devices/console-[0-9]", RUN+="xenconsole-setup-tty”

I think this should be documented somewhere in the patch, at the very
least.  Better would be to submit it upstream.

Ian.

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

Re: xl, libxl: add support for 'channels'

[Konrad Rzeszutek Wilk]

On Fri, Sep 26, 2014 at 04:14:16PM +0100, Ian Jackson wrote:
> Dave Scott writes ("Re: [Xen-devel] xl, libxl: add support for 'channels'"):
> > On 25 Sep 2014, at 20:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > > The major piece of information that is missing is - who/what generates
> > > the udev information in the guest? I could not find it in the Linux
> > > hvc_console.c driver so I am unclear of how it is suppose to
> > > work there?
> > 
> > I believe all secondary consoles (in device/console/ in xenstore)
> > generate the event. I’m not familiar with the kernel side of things;
> > Stefano: can you point us in the right direction?


Stefano?

Either way, I think this patch just need to mention what kernel driver
so that folks who are developing this can check that the required
kernel driver is loaded (or built).

> > 
> > For reference I used a udev rule to catch all secondary consoles:
> > 
> > # Set up secondary Xen consoles
> > SUBSYSTEM=="xen", DEVPATH=="/devices/console-[0-9]", RUN+="xenconsole-setup-tty”
> 
> I think this should be documented somewhere in the patch, at the very
> least.  Better would be to submit it upstream.

+1 in the patch. 

Upstream being in the libvirt side of the world? Sure - the more the merrier.


> 
> Ian.

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

Re: xl, libxl: add support for 'channels'

[Dave Scott]

On 26 Sep 2014, at 20:20, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Fri, Sep 26, 2014 at 04:14:16PM +0100, Ian Jackson wrote:
>> Dave Scott writes ("Re: [Xen-devel] xl, libxl: add support for 'channels'"):
>>> On 25 Sep 2014, at 20:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>>>> The major piece of information that is missing is - who/what generates
>>>> the udev information in the guest? I could not find it in the Linux
>>>> hvc_console.c driver so I am unclear of how it is suppose to
>>>> work there?
>>> 
>>> I believe all secondary consoles (in device/console/ in xenstore)
>>> generate the event. I’m not familiar with the kernel side of things;
>>> Stefano: can you point us in the right direction?
> 
> 
> Stefano?
> 
> Either way, I think this patch just need to mention what kernel driver
> so that folks who are developing this can check that the required
> kernel driver is loaded (or built).

I checked by deliberately mangling a xenstore key to see where the Linux errors
came from. The logged messages were from “xenconsole” and I got a stack
trace with

  xencons_disconnect_backend
  xencons_probe
  xenbus_dev_probe
  xenbus_frontend_dev_probe
  ..
  xenbus_register_driver_common
  xenbus_register_frontend
  ..
  xen_hvc_init

So it looks like the driver is hvc_xen.c, so people would need
CONFIG_HVC_XEN_FRONTEND. I’ll add that to the patch.


>>> 
>>> For reference I used a udev rule to catch all secondary consoles:
>>> 
>>> # Set up secondary Xen consoles
>>> SUBSYSTEM=="xen", DEVPATH=="/devices/console-[0-9]", RUN+="xenconsole-setup-tty”
>> 
>> I think this should be documented somewhere in the patch, at the very
>> least.  Better would be to submit it upstream.
> 
> +1 in the patch. 
> 
> Upstream being in the libvirt side of the world? Sure - the more the merrier.

:-) My plan is to upstream everything to the most appropriate places so

1. the protocol + API: this patch to libxl,xl,docs/misc (including details of the udev rule)
2. the udev rule itself: to wherever the distros get their udev rules from
3. the libvirt glue: to libvir-devel
4. application-level code to make use of it: to various places

I think the best ordering is to get the foundation ready first (i.e. this patch)
and then promote the rest. It would be risky to push dependent patches anywhere else
until the API has actually been fully released.

I’m a bit torn on the timing: on the one hand I’d really like to get a released API that
I can build on. On the other hand this is definitely a new feature and perhaps
at this stage it’s better to focus on fixing bugs rather than introducing them!

Once I’ve finished a bit of xenstore patch re-reviewing, I can send another spin of
this. However I’m still missing Acks on key patches from the maintainers. So if
you or they would rather focus bandwidth elsewhere, let me know and I’ll hibernate the
patches for 4.6.

Cheers,
Dave

Re: xl, libxl: add support for 'channels'

[Konrad Rzeszutek Wilk]

On Tue, Oct 07, 2014 at 04:52:58PM +0000, Dave Scott wrote:
> 
> On 26 Sep 2014, at 20:20, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > On Fri, Sep 26, 2014 at 04:14:16PM +0100, Ian Jackson wrote:
> >> Dave Scott writes ("Re: [Xen-devel] xl, libxl: add support for 'channels'"):
> >>> On 25 Sep 2014, at 20:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >>>> The major piece of information that is missing is - who/what generates
> >>>> the udev information in the guest? I could not find it in the Linux
> >>>> hvc_console.c driver so I am unclear of how it is suppose to
> >>>> work there?
> >>> 
> >>> I believe all secondary consoles (in device/console/ in xenstore)
> >>> generate the event. I’m not familiar with the kernel side of things;
> >>> Stefano: can you point us in the right direction?
> > 
> > 
> > Stefano?
> > 
> > Either way, I think this patch just need to mention what kernel driver
> > so that folks who are developing this can check that the required
> > kernel driver is loaded (or built).
> 
> I checked by deliberately mangling a xenstore key to see where the Linux errors
> came from. The logged messages were from “xenconsole” and I got a stack
> trace with
> 
>   xencons_disconnect_backend
>   xencons_probe
>   xenbus_dev_probe
>   xenbus_frontend_dev_probe
>   ..
>   xenbus_register_driver_common
>   xenbus_register_frontend
>   ..
>   xen_hvc_init
> 
> So it looks like the driver is hvc_xen.c, so people would need
> CONFIG_HVC_XEN_FRONTEND. I’ll add that to the patch.

Excellent. Thank you!
> 
> 
> >>> 
> >>> For reference I used a udev rule to catch all secondary consoles:
> >>> 
> >>> # Set up secondary Xen consoles
> >>> SUBSYSTEM=="xen", DEVPATH=="/devices/console-[0-9]", RUN+="xenconsole-setup-tty”
> >> 
> >> I think this should be documented somewhere in the patch, at the very
> >> least.  Better would be to submit it upstream.
> > 
> > +1 in the patch. 
> > 
> > Upstream being in the libvirt side of the world? Sure - the more the merrier.
> 
> :-) My plan is to upstream everything to the most appropriate places so
> 
> 1. the protocol + API: this patch to libxl,xl,docs/misc (including details of the udev rule)
> 2. the udev rule itself: to wherever the distros get their udev rules from
> 3. the libvirt glue: to libvir-devel
> 4. application-level code to make use of it: to various places
> 
> I think the best ordering is to get the foundation ready first (i.e. this patch)
> and then promote the rest. It would be risky to push dependent patches anywhere else
> until the API has actually been fully released.
> 
> I’m a bit torn on the timing: on the one hand I’d really like to get a released API that
> I can build on. On the other hand this is definitely a new feature and perhaps
> at this stage it’s better to focus on fixing bugs rather than introducing them!
> 
> Once I’ve finished a bit of xenstore patch re-reviewing, I can send another spin of
> this. However I’m still missing Acks on key patches from the maintainers. So if
> you or they would rather focus bandwidth elsewhere, let me know and I’ll hibernate the
> patches for 4.6.
> 
> Cheers,
> Dave
> 

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

Re: xl, libxl: add support for 'channels'

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3326 bytes --]

On Tue, 7 Oct 2014, Dave Scott wrote:
> On 26 Sep 2014, at 20:20, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > On Fri, Sep 26, 2014 at 04:14:16PM +0100, Ian Jackson wrote:
> >> Dave Scott writes ("Re: [Xen-devel] xl, libxl: add support for 'channels'"):
> >>> On 25 Sep 2014, at 20:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >>>> The major piece of information that is missing is - who/what generates
> >>>> the udev information in the guest? I could not find it in the Linux
> >>>> hvc_console.c driver so I am unclear of how it is suppose to
> >>>> work there?
> >>> 
> >>> I believe all secondary consoles (in device/console/ in xenstore)
> >>> generate the event. I’m not familiar with the kernel side of things;
> >>> Stefano: can you point us in the right direction?
> > 
> > 
> > Stefano?

Sorry for not reading the thread earlier: too many people CC me
nowadays, I didn't notice you ask me something :-/


> > Either way, I think this patch just need to mention what kernel driver
> > so that folks who are developing this can check that the required
> > kernel driver is loaded (or built).
> 
> I checked by deliberately mangling a xenstore key to see where the Linux errors
> came from. The logged messages were from “xenconsole” and I got a stack
> trace with
> 
>   xencons_disconnect_backend
>   xencons_probe
>   xenbus_dev_probe
>   xenbus_frontend_dev_probe
>   ..
>   xenbus_register_driver_common
>   xenbus_register_frontend
>   ..
>   xen_hvc_init
> 
> So it looks like the driver is hvc_xen.c, so people would need
> CONFIG_HVC_XEN_FRONTEND. I’ll add that to the patch.
> 
> 
> >>> 
> >>> For reference I used a udev rule to catch all secondary consoles:
> >>> 
> >>> # Set up secondary Xen consoles
> >>> SUBSYSTEM=="xen", DEVPATH=="/devices/console-[0-9]", RUN+="xenconsole-setup-tty”
> >> 
> >> I think this should be documented somewhere in the patch, at the very
> >> least.  Better would be to submit it upstream.
> > 
> > +1 in the patch. 
> > 
> > Upstream being in the libvirt side of the world? Sure - the more the merrier.
> 
> :-) My plan is to upstream everything to the most appropriate places so
> 
> 1. the protocol + API: this patch to libxl,xl,docs/misc (including details of the udev rule)
> 2. the udev rule itself: to wherever the distros get their udev rules from
> 3. the libvirt glue: to libvir-devel
> 4. application-level code to make use of it: to various places
> 
> I think the best ordering is to get the foundation ready first (i.e. this patch)
> and then promote the rest. It would be risky to push dependent patches anywhere else
> until the API has actually been fully released.
> 
> I’m a bit torn on the timing: on the one hand I’d really like to get a released API that
> I can build on. On the other hand this is definitely a new feature and perhaps
> at this stage it’s better to focus on fixing bugs rather than introducing them!
> 
> Once I’ve finished a bit of xenstore patch re-reviewing, I can send another spin of
> this. However I’m still missing Acks on key patches from the maintainers. So if
> you or they would rather focus bandwidth elsewhere, let me know and I’ll hibernate the
> patches for 4.6.
> 
> Cheers,
> Dave
> 

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: xl, libxl: add support for 'channels'

[Ian Campbell]

On Tue, 2014-10-07 at 17:52 +0100, Dave Scott wrote:
> >>> For reference I used a udev rule to catch all secondary consoles:
> >>> 
> >>> # Set up secondary Xen consoles
> >>> SUBSYSTEM=="xen", DEVPATH=="/devices/console-[0-9]", RUN+="xenconsole-setup-tty”
> >> 
> >> I think this should be documented somewhere in the patch, at the very
> >> least.  Better would be to submit it upstream.
> > 
> > +1 in the patch. 
> > 
> > Upstream being in the libvirt side of the world? Sure - the more the merrier.
> 
> :-) My plan is to upstream everything to the most appropriate places so
> 
> 1. the protocol + API: this patch to libxl,xl,docs/misc (including details of the udev rule)
> 2. the udev rule itself: to wherever the distros get their udev rules from

udev upstream, I think? We also ship some in xen.git but they are more
for backend stuff (which makes sense). For guest side I think udev
upstream is probably the most appropriate place since you want it
everywhere.

> 3. the libvirt glue: to libvir-devel
> 4. application-level code to make use of it: to various places
> 
> I think the best ordering is to get the foundation ready first (i.e. this patch)
> and then promote the rest. It would be risky to push dependent patches anywhere else
> until the API has actually been fully released.

I fully agree with your sequencing.

> I’m a bit torn on the timing: on the one hand I’d really like to get a released API that
> I can build on. On the other hand this is definitely a new feature and perhaps
> at this stage it’s better to focus on fixing bugs rather than introducing them!
> 
> Once I’ve finished a bit of xenstore patch re-reviewing, I can send another spin of
> this. However I’m still missing Acks on key patches from the maintainers. So if
> you or they would rather focus bandwidth elsewhere, let me know and I’ll hibernate the
> patches for 4.6.

I've quickly glanced through the review of this and most of it looked
pretty minor, perhaps it would be worth spinning up the version with
those fixed for consideration?

The most major comment I saw was this one from Ian J which Wei reposted
as:

        Ian Jackson asked one question which remained unanswered in the last
        iteration.
        
        QUOTE
        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.
        ENDQUOTE
        
        Has that question become moot? Did I miss anything?
        
        Wei.


> 
> Cheers,
> Dave
> 



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