PATCH RFC: possible implementation of a low-bandwidth private 'channel'

PATCH RFC: possible implementation of a low-bandwidth private 'channel'

[David Scott]

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 = [ "type=socket,path=/tmp/mysock,name=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).

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

[PATCH RFC 1/4] libxl: add a list of abstract 'channels' to the domain config

[David Scott]

A 'channel' is a low-bandwidth private communication channel that
resembles a physical serial port. Example uses include:

  * providing initial VM configuration without having to use the
    network
  * signalling a guest agent

Every channel has a string 'name' which the VM can use to find
the appropriate handler. Each channel has an implementation 'type'
which currently includes:

  * NONE: reads will block, writes will be thrown away
  * PTY: the I/O surfaces as a pty in the backend domain
  * PATH: writes are appended to a log file in the backend domain
  * SOCKET: a listening Unix domain socket accepts a connection in
    the backend domain and proxies

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

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 52f1aa9..4df8d42 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -51,6 +51,17 @@ libxl_domain_type = Enumeration("domain_type", [
     (2, "PV"),
     ], init_val = -1)
 
+libxl_channel_type = Enumeration("channel_type", [
+    # Connected to nothing:
+    (0, "NONE"),
+    # Connect to a pty in the backend domain:
+    (1, "PTY"),
+    # Spool output to a file in the backend domain:
+    (2, "PATH"),
+    # Listen on a Unix domain socket in the backend domain:
+    (3, "SOCKET"),
+    ], init_val = 0)
+
 libxl_device_model_version = Enumeration("device_model_version", [
     (0, "UNKNOWN"),
     (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
@@ -457,6 +468,15 @@ 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),
+    ("type", libxl_channel_type),
+    ("path", string),
+])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
@@ -467,6 +487,7 @@ libxl_domain_config = Struct("domain_config", [
     ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
     ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
     ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
+    ("channels", Array(libxl_device_channel, "num_channels")),
 
     ("on_poweroff", libxl_action_on_shutdown),
     ("on_reboot", libxl_action_on_shutdown),
-- 
1.7.10.4

[PATCH RFC 2/4] xl: add support for channels

[David Scott]

This adds support for channel declarations of the form:

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

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

If 'type = none' then the channel is connected to /dev/null
If 'type = pty' then the channel is connected to a pty in the
  backend domain
If 'type = path' then data is read from the channel and written
  to the file given by 'path = ...' in the backend domain.
If 'type = socket' then the channel is connected to a Unix domain
  socket given by 'path = ...' in the backend domain.

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5195914..2657b5d 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -736,7 +736,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;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 0;
@@ -1289,6 +1289,66 @@ 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 *channel;
+            char *buf2 = strdup(buf);
+            char *p, *p2;
+            d_config->channels = (libxl_device_channel *) realloc(d_config->channels, sizeof (libxl_device_channel) * (d_config->num_channels + 1));
+            channel = d_config->channels + d_config->num_channels;
+            libxl_device_channel_init(channel);
+            channel->devid = d_config->num_channels;
+
+            p = strtok(buf2, ",");
+            if (!p)
+                goto skip_channel;
+            do {
+                while (*p == ' ')
+                    p++;
+                if ((p2 = strchr(p, '=')) == NULL)
+                    break;
+                *p2 = '\0';
+                if (!strcmp(p, "backend")) {
+                    free(channel->backend_domname);
+                    channel->backend_domname = strdup(p2 + 1);
+                } else if (!strcmp(p, "name")) {
+                    free(channel->name);
+                    channel->name = strdup(p2 + 1);
+                } else if (!strcmp(p, "type")) {
+                    if (channel->type != LIBXL_CHANNEL_TYPE_NONE) {
+                        fprintf(stderr, "a channel may have only one output,"
+                                        " skipping device\n");
+                        goto skip_channel;
+                    }
+                    if (!strcmp(p2 + 1, "none")) {
+                        channel->type = LIBXL_CHANNEL_TYPE_NONE;
+                    } else if (!strcmp(p2 + 1, "pty")) {
+                        channel->type = LIBXL_CHANNEL_TYPE_PTY;
+                    } else if (!strcmp(p2 + 1, "path")) {
+                        channel->type = LIBXL_CHANNEL_TYPE_PATH;
+                    } else if (!strcmp(p2 + 1, "socket")) {
+                        channel->type = LIBXL_CHANNEL_TYPE_SOCKET;
+                    } else {
+                        fprintf(stderr, "unknown channel type '%s',"
+                                        " skipping device\n", p2 + 1);
+                        goto skip_channel;
+                    }
+                } else if (!strcmp(p, "path")) {
+                    free(channel->path);
+                    channel->path = strdup(p2 + 1);
+                } else {
+                    fprintf(stderr, "unknown channel parameter '%s',"
+                                    " ignoring\n", p);
+                }
+            } while ((p = strtok(NULL, ",")) != NULL);
+skip_channel:
+            free(buf2);
+            d_config->num_channels++;
+        }
+    }
+
     if (!xlu_cfg_get_list (config, "vif", &nics, 0, 0)) {
         d_config->num_nics = 0;
         d_config->nics = NULL;
-- 
1.7.10.4

[PATCH RFC 3/4] libxl: implement channels via PV console rings attached to qemu

[David Scott]

We extend the (internal) console type with a 'name' (string)
which isn't used by the default built-in console 0.

For every channel we create a console, starting at index 1,
which is handled by the qemu 'chardev' mechanism (ie has
'output=chardev:libxl-channel%d' in xenstore)

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/libxl/libxl_create.c           |   60 ++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types_internal.idl |    1 +
 2 files changed, 61 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d015cf4..13a2a27 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -361,11 +361,60 @@ static int init_console_info(libxl__device_console *console, int dev_num)
     console->devid = dev_num;
     console->consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
     console->output = strdup("pty");
+    /* console->name is NULL on normal consoles. Only 'channels' when mapped
+       to consoles have a string name. */
     if (!console->output)
         return ERROR_NOMEM;
     return 0;
 }
 
+static int init_console_from_channel(libxl__gc *gc,
+                                     libxl__device_console *console,
+                                     int dev_num,
+                                     libxl_device_channel *channel)
+{
+    const char *chardev;
+    memset(console, 0x00, sizeof(libxl__device_console));
+    console->devid = dev_num;
+    console->consback = LIBXL__CONSOLE_BACKEND_IOEMU;
+    if (!channel->name){
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+                   "channel %d has no name", channel->devid);
+        return ERROR_INVAL;
+    }
+    console->name = strdup(channel->name);
+    console->backend_domid = channel->backend_domid;
+
+    switch (channel->type) {
+        case LIBXL_CHANNEL_TYPE_NONE:
+        case LIBXL_CHANNEL_TYPE_PTY:
+            /* No path is needed */
+            break;
+        case LIBXL_CHANNEL_TYPE_PATH:
+        case LIBXL_CHANNEL_TYPE_SOCKET:
+            if (!channel->path) {
+                LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+                           "channel %d has no path", channel->devid);
+                return ERROR_INVAL;
+            }
+            break;
+        default:
+            /* We've forgotten to add the clause */
+            LOG(ERROR, "%s: unknown channel type %d", __func__, channel->type);
+            return ERROR_INVAL;
+    }
+
+    /* Use qemu chardev for every channel */
+    chardev = libxl__sprintf(gc, "chardev:libxl-channel%d",
+                                 channel->devid);
+    if (!chardev) return ERROR_NOMEM;
+    console->output = strdup(chardev);
+    if (!console->output) return ERROR_NOMEM;
+
+    return 0;
+}
+
+
 int libxl__domain_build(libxl__gc *gc,
                         libxl_domain_config *d_config,
                         uint32_t domid,
@@ -1110,6 +1159,17 @@ 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;
+        ret = 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);
+        libxl__device_console_dispose(&console);
+    }
+
     switch (d_config->c_info.type) {
     case LIBXL_DOMAIN_TYPE_HVM:
     {
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index cb9444f..2a509a9 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -32,6 +32,7 @@ libxl__device_console = Struct("device_console", [
     ("devid", integer),
     ("consback", libxl__console_backend),
     ("output", string),
+    ("name", string),
     ])
 
 libxl__device_action = Enumeration("device_action", [
-- 
1.7.10.4

[PATCH RFC 4/4] libxl: spawn a qemu to implement channels

[David Scott]

Since every channel is mapped to a console device in xenstore
with 'output=chardev:libxl-channel%d', we need to tell qemu
to create the appropriate chardevs.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/libxl/libxl_create.c   |    3 ++-
 tools/libxl/libxl_dm.c       |   44 ++++++++++++++++++++++++++++++++++++++++--
 tools/libxl/libxl_internal.h |    3 ++-
 3 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 13a2a27..2d2fb55 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1210,7 +1210,8 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
 
         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);
 
         console.backend_domid = state->console_domid;
         libxl__device_console_add(gc, domid, &console, state);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 51ab2bf..3269577 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -394,8 +394,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, type;
     uint64_t ram_size;
+    const char *path;
 
     dm_args = flexarray_make(gc, 16, 1);
 
@@ -412,6 +413,39 @@ 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++) {
+        flexarray_append(dm_args, "-chardev");
+        type = guest_config->channels[i].type;
+        path = guest_config->channels[i].path;
+        switch (type) {
+            case LIBXL_CHANNEL_TYPE_NONE:
+                flexarray_append(dm_args,
+                                 libxl__sprintf(gc, "null,id=libxl-channel%d",
+                                                    i));
+                break;
+            case LIBXL_CHANNEL_TYPE_PTY:
+                flexarray_append(dm_args,
+                                 libxl__sprintf(gc, "pty,id=libxl-channel%d",
+                                                    i));
+                break;
+            case LIBXL_CHANNEL_TYPE_PATH:
+                flexarray_append(dm_args,
+                                 libxl__sprintf(gc, "file,id=libxl-channel%d,"
+                                                    "path=%s", i, path));
+                break;
+            case LIBXL_CHANNEL_TYPE_SOCKET:
+                flexarray_append(dm_args,
+                                 libxl__sprintf(gc, "socket,id=libxl-channel%d,"
+                                                    "path=%s,server,nowait",
+                                                    i, path));
+                break;
+            default:
+                /* We've forgotten to add the clause */
+                LOG(ERROR, "%s: unknown channel type %d", __func__, type);
+                return NULL;
+        }
+    }
+
     /*
      * Remove default devices created by qemu. Qemu will create only devices
      * defined by xen, since the devices not defined by xen are not usable.
@@ -1517,7 +1551,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)
 {
     int i, ret = 0;
     uint32_t domid;
@@ -1557,6 +1592,11 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
         }
     }
 
+    if (nr_channels > 0) {
+        ret = 1;
+        goto out;
+    }
+
 out:
     return ret;
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 082749e..a3b1f57 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1450,7 +1450,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);
 
 /*
  * This function will cause the whole libxl process to hang
-- 
1.7.10.4

Re: [PATCH RFC 1/4] libxl: add a list of abstract 'channels' to the domain config

[Wei Liu]

On Wed, Jun 11, 2014 at 09:24:26PM +0100, David Scott wrote:
> A 'channel' is a low-bandwidth private communication channel that
> resembles a physical serial port. Example uses include:
> 
>   * providing initial VM configuration without having to use the
>     network
>   * signalling a guest agent
> 
> Every channel has a string 'name' which the VM can use to find
> the appropriate handler. Each channel has an implementation 'type'
> which currently includes:
> 
>   * NONE: reads will block, writes will be thrown away
>   * PTY: the I/O surfaces as a pty in the backend domain
>   * PATH: writes are appended to a log file in the backend domain
>   * SOCKET: a listening Unix domain socket accepts a connection in
>     the backend domain and proxies
> 
> Signed-off-by: David Scott <dave.scott@citrix.com>
> ---
>  tools/libxl/libxl_types.idl |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 52f1aa9..4df8d42 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -51,6 +51,17 @@ libxl_domain_type = Enumeration("domain_type", [
>      (2, "PV"),
>      ], init_val = -1)
>  
> +libxl_channel_type = Enumeration("channel_type", [
> +    # Connected to nothing:
> +    (0, "NONE"),
> +    # Connect to a pty in the backend domain:
> +    (1, "PTY"),
> +    # Spool output to a file in the backend domain:
> +    (2, "PATH"),
> +    # Listen on a Unix domain socket in the backend domain:
> +    (3, "SOCKET"),
> +    ], init_val = 0)
> +

You can omit init_val if it is 0.

Wei.

Re: [PATCH RFC 2/4] xl: add support for channels

[Wei Liu]

On Wed, Jun 11, 2014 at 09:24:27PM +0100, David Scott wrote:
> This adds support for channel declarations of the form:
> 
>   channel = [ "name=...,type=...[,path=...][,backend=...]" ]
> 
> where 'name' is a label to identify the channel to the frontend.
> 
> If 'type = none' then the channel is connected to /dev/null
> If 'type = pty' then the channel is connected to a pty in the
>   backend domain
> If 'type = path' then data is read from the channel and written
>   to the file given by 'path = ...' in the backend domain.
> If 'type = socket' then the channel is connected to a Unix domain
>   socket given by 'path = ...' in the backend domain.
> 
> Signed-off-by: David Scott <dave.scott@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |   62 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 61 insertions(+), 1 deletion(-)
> 

You also need to document this new config option in manpage.

And possibly the spec of defining channel should be documented too.

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5195914..2657b5d 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -736,7 +736,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;
>      int pci_power_mgmt = 0;
>      int pci_msitranslate = 0;
> @@ -1289,6 +1289,66 @@ 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) {

Line too long.

> +            libxl_device_channel *channel;
> +            char *buf2 = strdup(buf);
> +            char *p, *p2;
> +            d_config->channels = (libxl_device_channel *) realloc(d_config->channels, sizeof (libxl_device_channel) * (d_config->num_channels + 1));

Ditto.

And please use xrealloc instead, which handles allocation failure for you.

Wei.

Re: [PATCH RFC 3/4] libxl: implement channels via PV console rings attached to qemu

[Wei Liu]

On Wed, Jun 11, 2014 at 09:24:28PM +0100, David Scott wrote:
> We extend the (internal) console type with a 'name' (string)
> which isn't used by the default built-in console 0.
> 
> For every channel we create a console, starting at index 1,
> which is handled by the qemu 'chardev' mechanism (ie has
> 'output=chardev:libxl-channel%d' in xenstore)
> 
> Signed-off-by: David Scott <dave.scott@citrix.com>
> ---
>  tools/libxl/libxl_create.c           |   60 ++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_types_internal.idl |    1 +
>  2 files changed, 61 insertions(+)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index d015cf4..13a2a27 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -361,11 +361,60 @@ static int init_console_info(libxl__device_console *console, int dev_num)
>      console->devid = dev_num;
>      console->consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
>      console->output = strdup("pty");
> +    /* console->name is NULL on normal consoles. Only 'channels' when mapped
> +       to consoles have a string name. */
>      if (!console->output)
>          return ERROR_NOMEM;
>      return 0;
>  }
>  
> +static int init_console_from_channel(libxl__gc *gc,
> +                                     libxl__device_console *console,
> +                                     int dev_num,
> +                                     libxl_device_channel *channel)
> +{
> +    const char *chardev;
> +    memset(console, 0x00, sizeof(libxl__device_console));

I suppose you should use libxl__device_console_init instead?

> +    console->devid = dev_num;
> +    console->consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> +    if (!channel->name){
> +        LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> +                   "channel %d has no name", channel->devid);
> +        return ERROR_INVAL;
> +    }
> +    console->name = strdup(channel->name);

libxl__strdup, use NOGC if you don't want console->name gets freed.

> +    console->backend_domid = channel->backend_domid;
> +
> +    switch (channel->type) {
> +        case LIBXL_CHANNEL_TYPE_NONE:
> +        case LIBXL_CHANNEL_TYPE_PTY:
> +            /* No path is needed */
> +            break;
> +        case LIBXL_CHANNEL_TYPE_PATH:
> +        case LIBXL_CHANNEL_TYPE_SOCKET:
> +            if (!channel->path) {
> +                LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> +                           "channel %d has no path", channel->devid);
> +                return ERROR_INVAL;
> +            }
> +            break;
> +        default:
> +            /* We've forgotten to add the clause */
> +            LOG(ERROR, "%s: unknown channel type %d", __func__, channel->type);
> +            return ERROR_INVAL;
> +    }
> +
> +    /* Use qemu chardev for every channel */
> +    chardev = libxl__sprintf(gc, "chardev:libxl-channel%d",
> +                                 channel->devid);
> +    if (!chardev) return ERROR_NOMEM;
> +    console->output = strdup(chardev);

libxl__strdup might be a better choice, so that you can ...

> +    if (!console->output) return ERROR_NOMEM;

... get rid of this line.


Wei.

Re: [PATCH RFC 4/4] libxl: spawn a qemu to implement channels

[Wei Liu]

On Wed, Jun 11, 2014 at 09:24:29PM +0100, David Scott wrote:
[...]
> +    for (i = 0; i < guest_config->num_channels; i++) {
> +        flexarray_append(dm_args, "-chardev");
> +        type = guest_config->channels[i].type;
> +        path = guest_config->channels[i].path;
> +        switch (type) {
> +            case LIBXL_CHANNEL_TYPE_NONE:
> +                flexarray_append(dm_args,
> +                                 libxl__sprintf(gc, "null,id=libxl-channel%d",
> +                                                    i));

Wouldn't it be more appropriate to use channels[i].devid?

And the indentation here seems wrong.

There's also a macro called GCSPRINTF that you can use.

Wei.

Re: [PATCH RFC 4/4] libxl: spawn a qemu to implement channels

[Dave Scott]

Hi Wei,

On 12 Jun 2014, at 13:11, Wei Liu <wei.liu2@citrix.com> wrote:

> On Wed, Jun 11, 2014 at 09:24:29PM +0100, David Scott wrote:
> [...]
>> +    for (i = 0; i < guest_config->num_channels; i++) {
>> +        flexarray_append(dm_args, "-chardev");
>> +        type = guest_config->channels[i].type;
>> +        path = guest_config->channels[i].path;
>> +        switch (type) {
>> +            case LIBXL_CHANNEL_TYPE_NONE:
>> +                flexarray_append(dm_args,
>> +                                 libxl__sprintf(gc, "null,id=libxl-channel%d",
>> +                                                    i));
> 
> Wouldn't it be more appropriate to use channels[i].devid?
> 
> And the indentation here seems wrong.
> 
> There's also a macro called GCSPRINTF that you can use.

Thanks for taking a look -- I’ll make a v2 which fixes these problems.

Thanks,
Dave