Re: [Xen-devel] [PATCH v4 2/3] libxl: add support for 'channels'

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-07-22 at 16:05 +0100, David Scott wrote:
> diff --git a/docs/misc/console.txt b/docs/misc/console.txt
> index 8a53a95..eb08aff 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,47 @@ live in:
>  
>  /local/domain/$DOMID/device/console/$DEVID.
>  
> -The output of a PV console, whether it should be a file, a pty, a
> -socket, or something else, is specified by the toolstack in the xenstore
> -node "output", under the relevant console section.
> -For example:
> +PV consoles have
> +* (optional) string names;
> +* 'connection' information describing to what they should be
> +  connected; and
> +* a 'type' indicating which daemon should process the data.
> +
> +We call a PV console with a name a "channel", in reference to the libvirt
> +concept with the same name and purpose. The file "channels.txt" describes
> +how to use channels and includes a registry of well-known channel names.
> +
> +The toolstack writes 'connection' information in the xenstore backend in
> +nodes: "connection", "path" and "output". The nodes "connection" and "path"
> +express the high-level intention, while the "output" node is used for
> +internal signalling between the toolstack and the implementation daemon.

"connection" and "path" are both new, is that correct? Are they in any
way "channel", as opposed to serial, specific?

They correspond (roughly) to "method" and "method-specific-arguments" is
that right?

> +If the toolstack wants the console to be connected to a pty, it will write
> +to the backend:
> +
> +connection = pty
> +output = pty

Could we omit output in this case, since pty has no further parameters?

>  
> -# xenstore-read /local/domain/26/device/console/1/output
> -pty
> +The backend will write the pty device name to the "tty" node in the
> +console frontend.
>  
> -The backend chosen for a particular console is specified by the
> -toolstack in the "type" node on xenstore, under the relevant console
> -section.
> +If the toolstack wants a listening Unix domain socket to be created at path
> +<path>, a connection accepted and data proxied to the console, it will write:
> +
> +connection = socket
> +path = <path>
> +output = chardev:<some internal identifier>

Internal to whom? The toolstack? (this looks suspiciously like something
specific to the qemu backend)

I don't really understand the need for all three of these things,
especially how path and output are distinct. I have a feeling output may
be badly named and confusing me.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 3526539..769e6cf 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3299,6 +3299,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) {

It might be worth sanity checking that a supposedly primary console
doesn't have any properties which cannot be used with it?

> +        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));
>  
> @@ -3337,6 +3349,219 @@ out:
>  
>  
> /******************************************************************************/
>  
> +int libxl__init_console_from_channel(libxl__gc *gc,
> +                                     libxl__device_console *console,
> +                                     int dev_num,
> +                                     libxl_device_channel *channel)

Can channel be const?

> +{
> +    int rc;
> +    libxl__device_console_init(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 = 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:
> +            LIBXL__LOG(CTX, LIBXL__LOG_ERROR,

The LOG* macro family is useful to reduce the length of these lines.

> +                       "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) {
> +                LIBXL__LOG(CTX, LIBXL__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);
> +            return ERROR_INVAL;

Just a plain abort() is fine here.

> +    }
> +
> +    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 */

Please refactor it to the top of the file or something.

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

Stray tab.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index a412f9c..fcaf32f 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl

The new interface looks good to me.

Ian.


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