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