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

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.

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!