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@xxxxxxxxxx>
>>> ---
>>> 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);
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list