Re: [libvirt] [PATCH v3 2/4] libxl: channels support

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(a)oracle.com&gt; >> --- >> 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?

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.

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