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

[Eric Blake]

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org