On 2/19/19 3:33 PM, Eric Blake wrote: > 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. > I'm fine keeping away from the public doc comment about this. If (so far) no one has noticed, then why call attention to it. In the long run, I would think it's more important for a libvirt developer to not lose sight of the issue as opposed to a developer on libvirt. When/if the new flag is added whomever adds it would hopefully read the comments and if they don't we can always hope whomever reviews it may ask. John
