Red Hat Bugzilla – Bug 827060
domain xml: omit duplicate listen attribute from graphics element if it contains listen subelement
Last modified: 2016-04-26 23:24:44 EDT
Description of problem:
omit duplicate listen attribute from graphics element if it contains listen subelement. This is a minor yet frustrating nuisance from administrator's POV.
Version-Release number of selected component (if applicable):
Steps to Reproduce:
1. set up domain xml with graphics element like this and save domain:
<graphics type='spice' port='5800' autoport='no'>
<listen type='address' address='::0'/>
2. virsh edit the domain xml again
address is defined in two places, if they don't match, changes to the xml are discarded
address is defined just in <graphics><listen address='$ADDR'/></graphics>, duplicate in listen='$ADDR' attribute is ommited
Sorry, we can't break back-compat. Input XML can use either position (and if both positions are given, they must match), but output XML must always output both positions in order to avoid breaking older tools that expected it in the older position.
However, there _is_ a patch pending upstream to make 'virsh edit' ask if you want to retry an edit rather than just discard it if you make a mistake such as modifying one but not both of the two positions.
David, are you saing that libvirt is autogenerating the second address and causing the failure itself?
BTW, with F17, I see the duplicate listen address, but libvirt does not complain and my changes take effect.
Ah, I see, you are modifying the listen attribute itself. If I do that, I see:
error: XML error: graphics listen attribute 127.0.0.1 must match address attribute of first listen element (found ::0)
(In reply to comment #1)
> Sorry, we can't break back-compat. Input XML can use either position (and
> if both positions are given, they must match), but output XML must always
> output both positions in order to avoid breaking older tools that expected
> it in the older position.
But then the very presence of <listen address='$OLDADDR'/> element breaks old tools that would pass an edited XML back to libvirt, doesn't it?
Before <listen> introduction:
libvirt->tool: <graphics listen="$OLDADDR"/>
tool->libvirt: <graphics listen="$NEWADDR"/>
With both listen="" and <listen/>:
libvirt->tool: <graphics listen="$OLDADDR"><listen addr="$OLDADDR"/>...
tool->libvirt: <graphics listen="$NEWADDR"><listen addr="$OLDADDR"/>...
The very need to manipulate <listen> contents (even by deleting them) is compatibility issue, ain't it?
(In reply to comment #2)
> David, are you saing that libvirt is autogenerating the second address and
> causing the failure itself?
No, libvirt always generates correct addresses.
(In reply to comment #4)
> Ah, I see, you are modifying the listen attribute itself. If I do that, I
> error: XML error: graphics listen attribute 127.0.0.1 must match address
> attribute of first listen element (found ::0)
I see it too. Error message is good but the whole cause of error is IMO unnecessary, see above why I think so.
By default, the parent attribute MUST be generated for the sake of older tools that did not know about the newer <listen> child element.
But perhaps I have a way forward: what if we add a _new_ flag to virDomainGetXMLDesc that tells libvirt to omit redundant information (such as the parent attribute) because we know that the tool parsing the XML is not old and doesn't care about back-compat. 'virsh edit' would then use this flag by default.
I'm going to reopen this BZ. I'm not sure what the correct answer is here, but at the very least the behavior needs to be examined and documented.
https://www.redhat.com/archives/libvir-list/2012-May/msg01183.html is v2 of the upstream patches to make 'virsh edit' retry instead of abort if you have an incomplete edit, as hinted in comment 1. We're waiting for v3 of that patch, but that patch alone is still insufficient, if I go with my proposal in comment 6 to add a new flag 'virsh dumpxml --minimal' to kill the redundancy in the first place, as well as teach 'virsh edit' to use --minimal by default.
I don't think we should add any kind of '--minimal' flag here. This is just a solution looking for a problem. There is no real harm having this redundancy and allowing generation of XML documents without it, will cause more problems than it solves.
> There is no real harm having this redundancy
I think there is, as described in detail in #c5.
Oh, if inconsistent, then the code should be honouring the legacy listen attribute.
> Oh, if inconsistent, then the code should be honouring the legacy listen attribute.
Seems too simplistic approach to me. In case of conflict of legacy and current options, I'd prefer to ignore the one which says to maintain status quo. This is not an issue for interactive work once Michal's patches are merged but it could be an issue for non-interactive work. (It smells to me with race condition though so I don't have strong opinion that my idea is really better.)
Whatever is done for this problem, note that the same "duplication" occurs in the network xml, e.g.:
<forward dev='em4' mode='private'>
and its current behavior is consistent with that of the listen address - if both copies exist and don't match, an error is thrown.
I agree with David that just allowing the legacy attribute to take precedence isn't the proper answer; if someone updated the value in the "new" location, that approach would lead to the desired change being silently discarded. At least right now the user is alerted to the ambiguity.
This duplication has been around for years now and I don't see it changing at this point... I agree it's annoying but it's difficult to solve in a back compatible way.