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

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(a)suse.com&gt; 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.