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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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.

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.

Jan

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]