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

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

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

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

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

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

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


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