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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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