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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]