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!
Cheers,
Joao
Regards,
Jim