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

[Joao Martins]

On 09/26/2016 05:27 PM, Jim Fehlig wrote:
> On 09/26/2016 09:30 AM, Joao Martins wrote:
>> On 09/26/2016 03:21 PM, Jim Fehlig wrote:
>>> On 09/25/2016 01:13 PM, Joao Martins wrote:
 On 09/25/2016 07:55 PM, Joao Martins wrote:
> On 09/24/2016 12:15 AM, Joao Martins wrote:
>> On September 23, 2016 11:12:00 PM GMT+01:00, Jim Fehlig 
>>  wrote:
>>> On 09/22/2016 01:53 PM, Joao Martins wrote:
 [snip]
  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?
 Or even better have cfg as the function argument instead to allow also 
 removing
 "ctx" argument. Both channelDir and ctx are stored in cfg. This way we 
 reduce
 the number of arguments plus allow future usage on other functions called 
 from
 libxlBuildDomainConfig.
>>> Yep, I think that is fine. We primarily want to avoid making
>>> libxlBuildDomainConfig difficult to call from the unit tests. I realize we 
>>> don't
>>> currently do that, but the eventual plan is to test the conversion of
>>> virDomainDef to libxl_domain_config. danpb did some initial work on that 
>>> quite
>>> some time ago, see commit 5da28f24.
>> Ah nice to know, I wasn't aware of that work. This cover letter
>> (https://www.redhat.com/archives/libvir-list/2014-May/msg01102.html) 
>> explains it
>> all too.
> 
> Some of those patches were pushed. I did some followup work
> 
> https://www.redhat.com/archives/libvir-list/2014-September/msg00180.html
> 
> which also resulted in some patches being pushed. But the meaty parts of the
> series that actually provide the conversion tests were never committed. The 
> last
> attempt to revive the work also stalled
> 
>  https://www.redhat.com/archives/libvir-list/2015-January/msg00924.html
> 


> Thinking about it again, I seems the best way forward is something along the
> lines of option 3 described in that thread
> 
> "make use of new functionality in Xen 4.5 such as
> libxl_domain_config_from_json() and libxl_domain_config_compare()"
Interesting, the latter though (libxl_domain_config_compare) doesn't appear to
be implemented on 4.7 (or upcoming 4.8) and sounds like even if implemented that
it would rule out most of the versions :( Probably worked out with a shim for
older versions that implement equivalent of libxl_domain_config_compare().

>>  What I am in this patch is clearly opposite the effort of commit
>> 5da28f24 and a6abdbf. But now I am not sure if what I proposed in my earlier
>> paragrah is any different: we can probably get away with a mock of
>> libxlDriverConfig but not sure.
> 
> I suppose mocking it would be easier if libxl_ctx_alloc supported a dummy 
> mode.
> I.e. if this bug was fixed
> 
> https://bugs.xenproject.org/xen/bug/41
> 
>>  In the end sounds like just adding channelDir to
>> the function arguments might end up being better?
> 
> IMO that is probably the best approach until we have the conversion tests
> figured out.
Cool, thanks for the feedback! Let me submit v3 with these fixed.

> 
> BTW, can channels be hot (un)plugged? If so, it's another argument for just
> passing the channelDir. Future external callers of libxlMakeChannel() would 
> have
> access to the libxlDriverConfig object, and hence channelDir.
AFAICT there's no API for hotplugging channels, very much like serials/consoles
can't be too hotplugged.

Joao

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

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

[Joao Martins]

On 09/26/2016 03:21 PM, Jim Fehlig wrote:
> On 09/25/2016 01:13 PM, Joao Martins wrote:
>> On 09/25/2016 07:55 PM, Joao Martins wrote:
>>> On 09/24/2016 12:15 AM, Joao Martins wrote:
 On September 23, 2016 11:12:00 PM GMT+01:00, Jim Fehlig  
 wrote:
> On 09/22/2016 01:53 PM, Joao Martins wrote:
>> [snip]
>>  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?
>> Or even better have cfg as the function argument instead to allow also 
>> removing
>> "ctx" argument. Both channelDir and ctx are stored in cfg. This way we reduce
>> the number of arguments plus allow future usage on other functions called 
>> from
>> libxlBuildDomainConfig.
> 
> Yep, I think that is fine. We primarily want to avoid making
> libxlBuildDomainConfig difficult to call from the unit tests. I realize we 
> don't
> currently do that, but the eventual plan is to test the conversion of
> virDomainDef to libxl_domain_config. danpb did some initial work on that quite
> some time ago, see commit 5da28f24.

Ah nice to know, I wasn't aware of that work. This cover letter
(https://www.redhat.com/archives/libvir-list/2014-May/msg01102.html) explains it
all too. What I am in this patch is clearly opposite the effort of commit
5da28f24 and a6abdbf. But now I am not sure if what I proposed in my earlier
paragrah is any different: we can probably get away with a mock of
libxlDriverConfig but not sure. In the end sounds like just adding channelDir to
the function arguments might end up being better?

Joao

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

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

[Jim Fehlig]

On 09/25/2016 01:13 PM, Joao Martins wrote:
> On 09/25/2016 07:55 PM, Joao Martins wrote:
>> On 09/24/2016 12:15 AM, Joao Martins wrote:
>>> On September 23, 2016 11:12:00 PM GMT+01:00, Jim Fehlig  
>>> wrote:
 On 09/22/2016 01:53 PM, Joao Martins wrote:
> [snip]
>  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?
> Or even better have cfg as the function argument instead to allow also 
> removing
> "ctx" argument. Both channelDir and ctx are stored in cfg. This way we reduce
> the number of arguments plus allow future usage on other functions called from
> libxlBuildDomainConfig.

Yep, I think that is fine. We primarily want to avoid making
libxlBuildDomainConfig difficult to call from the unit tests. I realize we don't
currently do that, but the eventual plan is to test the conversion of
virDomainDef to libxl_domain_config. danpb did some initial work on that quite
some time ago, see commit 5da28f24.

Regards,
Jim

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

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

[Joao Martins]

On 09/25/2016 07:55 PM, Joao Martins wrote:
> On 09/24/2016 12:15 AM, Joao Martins wrote:
>> On September 23, 2016 11:12:00 PM GMT+01:00, Jim Fehlig  
>> wrote:
>>> On 09/22/2016 01:53 PM, Joao Martins wrote:
 [snip]
  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?
Or even better have cfg as the function argument instead to allow also removing
"ctx" argument. Both channelDir and ctx are stored in cfg. This way we reduce
the number of arguments plus allow future usage on other functions called from
libxlBuildDomainConfig.

Joao

P.S Sorry for so many messages!

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

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

[Joao Martins]

On 09/24/2016 12:15 AM, Joao Martins wrote:
> On September 23, 2016 11:12:00 PM GMT+01:00, Jim Fehlig  
> 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//
>>>
>>> Whereas for libxl:
>>>
>>> channels/target/-
>>>
>>> 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
>>>  
>>>
>>>
>>>  
>>>
>>>  $ 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 
>>> ---
>>> 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(_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(>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",
>>> +  

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

[Joao Martins]

On September 23, 2016 11:12:00 PM GMT+01:00, Jim Fehlig  
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//
>>
>> Whereas for libxl:
>>
>> channels/target/-
>>
>> 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
>>  
>>
>>
>>  
>>
>>  $ 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 
>> ---
>> 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(_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(>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:
>> +

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

[Jim Fehlig]

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//
>
> Whereas for libxl:
>
> channels/target/-
>
> 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
>  
>
>
>  
>
>  $ 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 
> ---
> 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(_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(>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.

> +
> +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,
> +  

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

[Joao Martins]

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

Whereas for libxl:

channels/target/-

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
 
   
   
 

 $ 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 
---
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(_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(>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;
+
+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"));
+