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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
