Re: [libvirt] [PATCH v2 1/5] domain: Fix unknown flags diagnosis in virDomainGetXMLDesc

On 2/19/19 2:24 PM, Eric Blake wrote: >> Since this is limited to the GetXMLDesc processing wouldn't this limit >> the exposure in such a way that some new flag expecting some default >> action would not return expected or "new" results on the newer libvirt >> vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS >> (SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that >> list, the get wouldn't fail because it doesn't know the flag, but it >> also wouldn't return any XML related to the new flag, right? > > Indeed - and that's how I found it: I'm trying to add > VIR_DOMAIN_XML_SNAPSHOTS and VIR_DOMAIN_XML_CHECKPOINTS flags, and was > confused when I patched virsh to support the new flag but the old > libvirt didn't react to it in any way (I then confirmed that libvirt > running on RHEL 6 _does_ react to the unknown flag). Missing output is > not a security breach, but is annoying enough to be worth comments > somewhere as to the fact that the problem exists (worse would be if we > wanted to add a flag comparable in nature to VIR_DOMAIN_XML_SECURE, > where failure to obey the flag results in leaking XML information that > should have been suppressed). > >> >> Secondary to that knowing this would require someone to read this >> specific commit message to garnish at least a small understanding of the >> issue. Perhaps some comments in domain_conf.h near the new >> VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand >> those without understanding the impact. Similarly, near the flags >> definition either a similar noteworthy comment or a comment to go read >> the domain_conf.h comment. There's a quick comment added before >> virDomainDefFormatConvertXMLFlags in this patch that could be expandable >> instead. Doesn't mean someone will read and/or understand it, but at >> least leaves a trail. > > Sure, I can add some strategic comments. Hmm - is there an easy way to add comments to the public include/ headers that are useful to developers, but which don't pollute the generated documentation? Or is this the sort of fix where a public doc comment that "some older versions of libvirt failed to diagnose unknown flags" is acceptable? Comments in domain_conf.[ch] are much easier as they don't affect the public docs.