On 09/26/2016 04:48 PM, Joao Martins wrote:
On 09/26/2016 10:44 PM, Jim Fehlig wrote:
On 09/26/2016 11:33 AM, Joao Martins wrote:
And allow libxl to handle channel element which creates a Xen console visible to the guest as a low-bandwitdh communication channel. If type is PTY we also fetch the tty after boot using libxl_channel_getinfo to fetch the tty path. On socket case, we autogenerate a path if not specified in the XML. Path autogenerated is slightly different from qemu driver: qemu stores also on "channels/target" but it creates then a directory per domain with each channel target name. libxl doesn't appear to have a clear definition of private files associated with each domain, so for simplicity we do it slightly different. On qemu each autogenerated channel goes like:
channels/target/<domain-name>/<target name>
Whereas for libxl:
channels/target/<domain-name>-<target name>
Should note that if path is not specified it won't persist, existing only on live XML, unless user had initially specified it. Since support for libxl channels only came on Xen >= 4.5 we therefore need to conditionally compile it with LIBXL_HAVE_DEVICE_CHANNEL.
After this patch and having a qemu guest agent: $ cat domain.xml | grep -a1 channel | head -n 5 | tail -n 4 <channel type='unix'> <source mode='bind' path='/tmp/channel'/> <target type='xen' name='org.qemu.guest_agent.0'/> </channel>
$ virsh create domain.xml $ echo '{"execute":"guest-network-get-interfaces"}' | socat stdio,ignoreeof unix-connect:/tmp/channel
{"execute":"guest-network-get-interfaces"} {"return": [{"name": "lo", "ip-addresses": [{"ip-address-type": "ipv4", "ip-address": "127.0.0.1", "prefix": 8}, {"ip-address-type": "ipv6", "ip-address": "::1", "prefix": 128}], "hardware-address": "00:00:00:00:00:00"}, {"name": "eth0", "ip-addresses": [{"ip-address-type": "ipv4", "ip-address": "10.100.0.6", "prefix": 24}, {"ip-address-type": "ipv6", "ip-address": "fe80::216:3eff:fe40:88eb", "prefix": 64}], "hardware-address": "00:16:3e:40:88:eb"}, {"name": "sit0"}]}
Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- Since v2: * Remove ret variable and do similar to other make functions. * Have channelDir passed as an argument instead of driver.
Since v1: * Autogenerated paths, and updated commit message explaning it the different naming. Despite per domain name being slightly different, parent directory is same across both drivers. --- src/libxl/libxl_conf.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_conf.h | 3 ++ src/libxl/libxl_domain.c | 43 +++++++++++++++++- src/libxl/libxl_driver.c | 7 +++ 4 files changed, 162 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 92faa44..4c94d54 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -88,6 +88,7 @@ libxlDriverConfigDispose(void *obj) VIR_FREE(cfg->saveDir); VIR_FREE(cfg->autoDumpDir); VIR_FREE(cfg->lockManagerName); + VIR_FREE(cfg->channelDir); virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares); }
@@ -1348,6 +1349,8 @@ libxlDriverConfigNew(void) goto error; if (VIR_STRDUP(cfg->autoDumpDir, LIBXL_DUMP_DIR) < 0) goto error; + if (VIR_STRDUP(cfg->channelDir, LIBXL_CHANNEL_DIR) < 0) + goto error;
if (virAsprintf(&log_file, "%s/libxl-driver.log", cfg->logDir) < 0) goto error; @@ -1499,6 +1502,107 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
}
+#ifdef LIBXL_HAVE_DEVICE_CHANNEL +static int +libxlPrepareChannel(virDomainChrDefPtr channel, + const char *channelDir, + const char *domainName) +{ + if (channel->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN && + channel->source.type == VIR_DOMAIN_CHR_TYPE_UNIX && + !channel->source.data.nix.path) { + if (virAsprintf(&channel->source.data.nix.path, + "%s/%s-%s", channelDir, domainName, + channel->target.name ? channel->target.name + : "unknown.sock") < 0) + return -1; + + channel->source.data.nix.listen = true; + } + + return 0; +} + +static int +libxlMakeChannel(virDomainChrDefPtr l_channel, + libxl_device_channel *x_channel) +{ + libxl_device_channel_init(x_channel); + + if (l_channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("channel target type not supported")); + return -1; + } + + switch (l_channel->source.type) { + case VIR_DOMAIN_CHR_TYPE_PTY: + x_channel->connection = LIBXL_CHANNEL_CONNECTION_PTY; + break; + case VIR_DOMAIN_CHR_TYPE_UNIX: + x_channel->connection = LIBXL_CHANNEL_CONNECTION_SOCKET; + if (VIR_STRDUP(x_channel->u.socket.path, + l_channel->source.data.nix.path) < 0) + return -1; + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("channel source type not supported")); + break; + } + + if (!l_channel->target.name) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("channel target name missing")); + return -1; + } + + if (VIR_STRDUP(x_channel->name, l_channel->target.name) < 0) + return -1; + + return 0; +} + +static int +libxlMakeChannelList(const char *channelDir, + virDomainDefPtr def, + libxl_domain_config *d_config) +{ + virDomainChrDefPtr *l_channels = def->channels; + size_t nchannels = def->nchannels; + libxl_device_channel *x_channels; + size_t i, nvchannels = 0; + + if (VIR_ALLOC_N(x_channels, nchannels) < 0) + return -1; + + for (i = 0; i < nchannels; i++) { + if (l_channels[i]->deviceType != VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) + continue; + + if (libxlPrepareChannel(l_channels[i], channelDir, def->name) < 0) + goto error; + + if (libxlMakeChannel(l_channels[i], &x_channels[nvchannels]) < 0) + goto error; + + nvchannels++; + } + + VIR_SHRINK_N(x_channels, nchannels, nchannels - nvchannels); + d_config->channels = x_channels; + d_config->num_channels = nvchannels; + + return 0; + + error: + for (i = 0; i < nchannels; i++) + libxl_device_channel_dispose(&x_channels[i]); + VIR_FREE(x_channels); + return -1; +} +#endif + #ifdef LIBXL_HAVE_PVUSB int libxlMakeUSBController(virDomainControllerDefPtr controller, @@ -1839,6 +1943,7 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, + const char *channelDir, libxl_ctx *ctx, libxl_domain_config *d_config) { @@ -1873,6 +1978,11 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, return -1; #endif
+#ifdef LIBXL_HAVE_DEVICE_CHANNEL + if (libxlMakeChannelList(channelDir, def, d_config) < 0) + return -1; +#endif + Sorry to report this fails to build if LIBXL_HAVE_DEVICE_CHANNEL is not defined Argh, sorry about this. In the rush of submitting this before beginning the freeze, I didn't remember to test the !LIBXL_HAVE_DEVICE_CHANNEL. Previous versions were good as it had driver being passed which was being also used to fetch reservedGraphicsPorts.
libxl/libxl_conf.c: In function 'libxlBuildDomainConfig': libxl/libxl_conf.c:1946:36: error: unused parameter 'channelDir' [-Werror=unused-parameter] const char *channelDir, ^ cc1: all warnings being treated as errors
Something like the below diff works and minimizes the spread of 'ifdef LIBXL_HAVE_DEVICE_CHANNEL'. Another solution is the approach take e.g. in src/network/bridge_driver.h. Or I'm open to other suggestions. Thanks, similar to src/network/bridge_driver.h you probably mean something like the diff below?
Yes.
It reuses the #ifdef LIBXL_HAVE_DEVICE_CHANNEL already in place in the patch . Though Your diff follow the general style with LIBXL_HAVE_* and potentially allows cleaner handling if we were to extend with more more arguments in libxlBuildDomainConfig or other function.
I'm on the fence, but your latter observation is enough to tip towards my diff. I've squashed it into this patch. Regards, Jim
FWIW, both approaches look good to me.
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 4c94d54..05745ee 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1601,6 +1601,14 @@ libxlMakeChannelList(const char *channelDir, VIR_FREE(x_channels); return -1; } +#else +static int +libxlMakeChannelList(const char *channelDir ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, + libxl_domain_config *d_config ATTRIBUTE_UNUSED) +{ + return 0; +} #endif
#ifdef LIBXL_HAVE_PVUSB @@ -1978,10 +1986,8 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, return -1; #endif
-#ifdef LIBXL_HAVE_DEVICE_CHANNEL if (libxlMakeChannelList(channelDir, def, d_config) < 0) return -1; -#endif
/* * Now that any potential VFBs are defined, update the build info with
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 4c94d54..1befd11 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1943,7 +1943,7 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir, + const char *channelDir LIBXL_ATTR_UNUSED, libxl_ctx *ctx, libxl_domain_config *d_config) { diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 2d60fcc..0ea76b4 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -198,10 +198,15 @@ libxlMakeUSB(virDomainHostdevDefPtr hostdev, libxl_device_usbdev *usbdev); virDomainXMLOptionPtr libxlCreateXMLConf(void);
+# ifdef LIBXL_HAVE_DEVICE_CHANNEL +# define LIBXL_ATTR_UNUSED +# else +# define LIBXL_ATTR_UNUSED ATTRIBUTE_UNUSED +# endif int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir, + const char *channelDir LIBXL_ATTR_UNUSED, libxl_ctx *ctx, libxl_domain_config *d_config);