Re: [Xen-devel] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 11/20/2015 05:40 PM, Joao Martins wrote:
>
> On 11/20/2015 07:05 PM, Jim Fehlig wrote:
>> On 11/19/2015 04:45 PM, Joao Martins wrote:
>>
>> You're not going to be happy with me...
>>
>>> This new field in libxlDomainObjPrivate is named "config"
>>> and is kept while the domain is active.
>> While this sounded like a good idea when I mentioned it, I'm now worried that
>> the config will quickly become stale and cause problems if used elsewhere 
>> (e.g.
>> see my yet-to-be-written comment in 3/3).  IIUC correctly, 
>> libxl_domain_config
>> is only useful when creating the domain. Subsequently adding/removing 
>> devices,
>> memory, vcpus, etc. would not be reflected in the libxl_domain_config 
>> object. I
>> suppose it would useful (and still valid) in the start callback, but IMO
>> including it in the libxlDomainPrivate struct fools us into believing it 
>> could
>> be used elsewhere throughout the life of the domain. I now have second doubts
>> about this. What do you think?
> I agree with you, and since there a libxl_device_nic_list as you suggested, it
> would actually be much cleaner and safer compared to libxl_domain_config
> alternative (though with a small performance cost). And we would avoid end up
> having config just lying there with no additional use (besides StartCallback)
> and inconsistent info.
>
> The only thing that the libxlDomainObjPrivate approach is better than
> libxl_device_nic_list() would be that we don't need to refetch the devid, 
> since
> the nics array has it correctly filled already when console callback is 
> invoked.
> Whereas libxl_device_nic_list will refetch the same info (in additiona to all
> entries in the backend directory) from xenstore thus adding up overhead. But
> given that this is only once and in domain create I think it's not a big deal.

Right. I think the extra overhead is in the noise relative to the other
activities involved in starting a domain.

> Would you agree then to resend this series without this patch and using
> libxl_device_nic_list, as the final approach? Thanks for pointing out this 
> issue!

I think so. If you dislike the extra overhead of libxl_device_nic_list, another
option would be something like a libxlDomainStartCallbackInfo struct that
contains the virDomainObj and libxl_domain_config, and is passed to the start
callback via for_callback of libxl_asyncop_how. That would allow us to use the
libxl_domain_config object in the callback, but still dispose it after the start
completes.

Regards,
Jim


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel