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

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

 



On 09/24/2016 12:15 AM, Joao Martins wrote:
> On September 23, 2016 11:12:00 PM GMT+01:00, Jim Fehlig <jfehlig@xxxxxxxx> wrote:
>> On 09/22/2016 01:53 PM, Joao Martins wrote:
>>> 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 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.
>>> * Remove the switch case from target type xen and rework the function
>> structure
>>> a bit.
>>> ---
>>>  src/libxl/libxl_conf.c   | 120
>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>>  src/libxl/libxl_conf.h   |   4 +-
>>>  src/libxl/libxl_domain.c |  44 ++++++++++++++++-
>>>  src/libxl/libxl_driver.c |   7 +++
>>>  4 files changed, 171 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index 306e441..824f2d2 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);
>>>  }
>>>  
>>> @@ -1339,6 +1340,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;
>>> @@ -1490,6 +1493,114 @@ 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)
>>> +{
>>> +    int ret = -1;
>>
>> Similar to the other libxlMake* functions, ret could be dropped simply
>> return -1
>> when encountering failure.
> 
> Ok, will change that.
> 
>>> +
>>> +    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 ret;
>>> +    }
>>> +
>>> +    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 ret;
>>> +        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 ret;
>>> +    }
>>> +
>>> +    if (VIR_STRDUP(x_channel->name, l_channel->target.name) < 0)
>>> +        return ret;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int
>>> +libxlMakeChannelList(libxlDriverPrivatePtr driver,
>>> +                     virDomainDefPtr def,
>>> +                     libxl_domain_config *d_config)
>>> +{
>>> +    virDomainChrDefPtr *l_channels = def->channels;
>>> +    size_t nchannels = def->nchannels;
>>> +    libxl_device_channel *x_channels;
>>> +    libxlDriverConfigPtr cfg;
>>> +    size_t i, nvchannels = 0;
>>> +
>>> +    if (VIR_ALLOC_N(x_channels, nchannels) < 0)
>>> +        return -1;
>>> +
>>> +    cfg = libxlDriverConfigGet(driver);
>>> +
>>> +    for (i = 0; i < nchannels; i++) {
>>> +        if (l_channels[i]->deviceType !=
>> VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL)
>>> +            continue;
>>> +
>>> +        if (libxlPrepareChannel(l_channels[i], cfg->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;
>>> +    virObjectUnref(cfg);
>>> +
>>> +    return 0;
>>> +
>>> + error:
>>> +    for (i = 0; i < nchannels; i++)
>>> +        libxl_device_channel_dispose(&x_channels[i]);
>>> +    VIR_FREE(x_channels);
>>> +    virObjectUnref(cfg);
>>> +    return -1;
>>> +}
>>> +#endif
>>> +
>>>  #ifdef LIBXL_HAVE_PVUSB
>>>  int
>>>  libxlMakeUSBController(virDomainControllerDefPtr controller,
>>> @@ -1828,11 +1939,13 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr
>> driver, virNodeInfoPtr info)
>>>  }
>>>  
>>>  int
>>> -libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>>> +libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
>>>                         virDomainDefPtr def,
>>>                         libxl_ctx *ctx,
>>>                         libxl_domain_config *d_config)
>>>  {
>>> +    virPortAllocatorPtr graphicsports =
>> driver->reservedGraphicsPorts;
>>> +
>>
>> Spurious change?
> 
> This (and the other two below) was intended, as I needed
>  channelDir. and instead of having yet another argument, I
>  passed driver instead as graphics port was using it too.
> 
> But I could use the macro directly, or add another argument if you prefer.

Hmm, I can just avoid passing driver and have cfg->channelDir added as an
argument. I just noticed that I am unnecessarily doing libxlDriverConfigGet
twice and perhaps if a third argument is added in the future then probably
consider having driver be passed as an argument?

Joao

--
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]