[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 <jfehlig at suse.com> 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
> 
<nods>

> 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