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