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

Regards,
Jim