|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 2/3] libxl: add support for 'channels'
Hi Ian,
On 10 Sep 2014, at 15:41, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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?
“connection” and “path” are indeed new, and yeah, they do correspond roughly to
“method” and “method-specific-arguments”.
The summary is:
- “connection” and “path” are the interface: a backend running libxl/xld (or
something else) could read these keys and create properly configured backends
- “output” is a pre-existing key used by libxl to signal qemu i.e. it’s part
of the secondary console implementation at the moment. If we enhanced
xenconsoled to support secondary consoles then we could drop this.
Initially I thought we could rely only on the pre-existing “output” key, which
is fine if “connection=output=pty” but in the case of a unix domain socket you
end up with:
connection = socket
path = /tmp/sock
output = chardev:libxl-1
where ‘chardev:libxl-1’ is an identifier that only appears on the qemu
commandline i.e. it’s not enough by itself for a driver domain implementation
to use. A driver domain implementation would read ‘connection’, ‘path’, and
synthesise the ‘output’ key and qemu command-line itself.
Since the doc describes itself as a design doc, I thought I should include some
of the gory detail. Perhaps I need to emphasise which bits are purely gory
implementation details.
>> +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?
Alas qemu needs ‘output = pty’. We could omit ‘connection = pty’ if we wanted,
although I thought it would be simpler to always write the configuration data
without cooking it too much.
>
>>
>> -# 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.
Perhaps if I labelled the ‘output’ key as a qemu implementation artefact which
should be ignored we could claim there are only 2 keys :-)
If we enhanced xenconsoled to support secondary consoles then we could favour
it over qemu and omit the ‘output’ key in those cases.
Thanks for the other comments — I’ll start investigating them.
Cheers,
Dave
>
>> 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |