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

[Jim Fehlig]

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

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()"

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

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.

Regards,
Jim