Re: [libvirt] [PATCH v2 1/7] dumpxml: add flag to virDomainGetXMLDesc

On 11/18/2014 07:50 AM, Adam Litke wrote: >> >> diff --git a/include/libvirt/libvirt-domain.h >> b/include/libvirt/libvirt-domain.h >> index 1fac2a3..caf1f46 100644 >> --- a/include/libvirt/libvirt-domain.h >> +++ b/include/libvirt/libvirt-domain.h >> @@ -1248,6 +1248,7 @@ typedef enum { >> VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain >> information */ >> VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU >> requirements according to host CPU */ >> VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for >> migration */ >> + VIR_DOMAIN_XML_BLOCK_INFO = (1 << 4), /* include storage volume >> information about each disk source */ > > I'll admit I'm not a master API designer but this is a red flag for me > (pun most definitely intended). Up to this point, the individual > virDomainXMLFlags are used to ask for XML for a certain purpose. For > example, VIR_DOMAIN_XML_INACTIVE retrieves XML suitable for a call to > virDomainCreateXML whereas VIR_DOMAIN_XML_MIGRATABLE is used to "dump > XML suitable for migration". Not quite. The VIR_DOMAIN_XML_SECURE and VIR_DOMAIN_XML_UPDATE_CPU flags both have the behavior of dumping additional information that is omitted by default. And the VIR_DOMAIN_XML_INACTIVE flag really means 'dump the xml that will be used on the next boot' if the domain is persistent (if the domain is transient, it says 'dump the xml that would be used on the next boot if I make the domain persistent'). > > This new flag is somewhat arbitrarily slicing up some of the ACTIVE VM > information. Slippery slope logic would lead us to a future API with > a proliferation of flags that turn various bits of info on and off > which would be very cumbersome to maintain and use. Since this > information is really ACTIVE VM info, I'd prefer it to be provided > whenever VIR_DOMAIN_XML_INACTIVE is not set. Callers who want a > tight XML document can always use VIR_DOMAIN_XML_INACTIVE. The thing is that this new flag is providing additional informative information that is NOT being stored as part of the domain itself, but which is being queried from the disks each time the xml is gathered. What's more, the information is valid for both active and inactive xml (it is not an active-only flag), and gathering the information potentially requires a monitor command for an active domain (none of the other XML information requires a monitor command). It is not good to require a monitor command by default, which is why I felt that having the flag opt-in to the behavior is the best way to preserve back-compat for callers that don't need the extra information.