Re: [libvirt] [PATCH 1/3] libxl: add libxl_domain_config to libxlDomainObjPrivate

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