[libvirt] [PATCH] Ignore listen attribute of <graphics> for type network listens

[Laine Stump]

On 02/26/2015 11:39 AM, Ján Tomko wrote:
> On Thu, Feb 26, 2015 at 04:29:53PM +0100, Martin Kletzander wrote:
>> On Thu, Feb 26, 2015 at 09:57:22AM -0500, Laine Stump wrote:
>>> On 02/26/2015 08:53 AM, Ján Tomko wrote:
>>>> Commit 6992994 started filling the listen attribute
>>>> of the parent <graphics> elements from type='network' listens.
>>>>
>>>> When this XML is passed to UpdateDevice, parsing fails:
>>>> XML error: graphics listen attribute 10.20.30.40 must match
>>>> address attribute of first listen element (found none)
>>>
>>> Note that the listen attribute of <graphics> won't be filled in if you
>>> request the *inactive* xml, and so there will be no error when it is fed
>>> back to updatedevice. I can't think of any examples right now, but have
>>> a very definite memory that there are several items in the config like
>>> this - if you feed the status xml output back to update/define "you're
>>> gonna have a bad time". That's why virsh edit asks for the INACTIVE xml.
>>>
>>> Did you see this when trying to do an update-device manually, or as the
>>> result of some management application that has forgotten to add the
>>> INACTIVE flag to its request for XML?
>>>
>>> If the latter, then I guess I can live with ignoring the error in order
>>> to preserve backward compatibility with the broken application.
>>> Reluctant ACK.
>>>
> Yes, this was a workaround for vdsm.
>
> But now I notice that  since the check is only done against type='address'
> listens, restarting libvirtd will lose any domain started with
> listen type='network', because libvirt fails to parse its own live XML.

Ah yes. Less reluctance (*much* less) in the ACK then. I actually ran
into something similar with networks just a week or two ago - commit
8f8e581a - and had to do the same thing.

"If you live in glass houses, don't throw stones" :-)


>
> I'll wait with pushing the patch just in case someone else wants to
> chime in.
>
>> I think we shouldn't fail parsing live XML when we were the ones who
>> generated it.  Take a look at device aliases for example.  We parse
>> them, but users aren't allowed to set them, so we don't parse them
>> when we are parsing inactive XML.  However, if I remember correctly,
>> we must be parsing them when parsing e.g. status XML.  So what if this
>> patch is modified so it behaves depending on flags?
>>
> With the INACTIVE flag (which is the case here in UpdateDevice), the
> listen addresses for <listen type='network'> subelements are already ignored.

... but we can't ignore the listen attribute in <graphics> because in
older XML (prior to the introduction of the <listen> subelement) that is
the only place to find the listen address.

You know, I hadn't even paid conscious attention to whether the INACTIVE
flag is set when calling parse functions before. Of course, in this case
it wouldn't make a difference, since we not only have to account for
reading our own status XML, but also for reading the XML sent by
"broken" applications.

>
> The INACTIVE flag is used for almost every domain definition parsing (at
> least in the QEMU driver - the only place with live XML parsing I found
> is the driver initialization).
>
> I can fix the verification to work against LISTEN_TYPE_NETWORK addresses
> too for live XML, but since:
> 1) we only parse live XML on driver initialization, so it should be
> generated by libvirt
> 2) even if we parsed a wrong listen address from there, it won't be used -
> virDomainGraphicsDefFormat gets it from the listens array.
>
> I chose the simpler patch with less breakage potential, since we're past
> rc2.

Agreed.