Bug 827060 - domain xml: omit duplicate listen attribute from graphics element if it contains listen subelement
domain xml: omit duplicate listen attribute from graphics element if it conta...
Status: CLOSED DEFERRED
Product: Virtualization Tools
Classification: Community
Component: libvirt (Show other bugs)
unspecified
Unspecified Unspecified
unspecified Severity medium
: ---
: ---
Assigned To: Libvirt Maintainers
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-31 10:22 EDT by David Jaša
Modified: 2016-04-26 23:24 EDT (History)
10 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-03-22 07:35:40 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description David Jaša 2012-05-31 10:22:00 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):
libvirt-0.9.10-20.el6.x86_64

How reproducible:
always

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'/>
    </graphics>
2. virsh edit the domain xml again
3.
  
Actual results:
address is defined in two places, if they don't match, changes to the xml are discarded

Expected results:
address is defined just in <graphics><listen address='$ADDR'/></graphics>, duplicate in listen='$ADDR' attribute is ommited

Additional info:
Comment 1 Eric Blake 2012-05-31 10:41:14 EDT
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.
Comment 2 Dave Allan 2012-05-31 10:47:16 EDT
David, are you saing that libvirt is autogenerating the second address and causing the failure itself?
Comment 3 Dave Allan 2012-05-31 10:50:36 EDT
BTW, with F17, I see the duplicate listen address, but libvirt does not complain and my changes take effect.
Comment 4 Dave Allan 2012-05-31 10:56:59 EDT
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)
Comment 5 David Jaša 2012-05-31 11:01:19 EDT
(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"/>
libvirt->tool: OK

With both listen="" and <listen/>:
libvirt->tool: <graphics listen="$OLDADDR"><listen addr="$OLDADDR"/>...
tool->libvirt: <graphics listen="$NEWADDR"><listen addr="$OLDADDR"/>...
libvirt->tool: PHAIL

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
> see:
> 
> 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.
Comment 6 Eric Blake 2012-05-31 11:05:17 EDT
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.
Comment 7 Dave Allan 2012-05-31 11:06:23 EDT
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.
Comment 8 Eric Blake 2012-05-31 13:52:42 EDT
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.
Comment 9 Daniel Berrange 2012-07-12 05:31:15 EDT
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.
Comment 10 David Jaša 2012-07-12 05:34:38 EDT
> There is no real harm having this redundancy

I think there is, as described in detail in #c5.
Comment 11 Daniel Berrange 2012-07-12 05:40:15 EDT
Oh, if inconsistent, then the code should be honouring the legacy listen attribute.
Comment 12 David Jaša 2012-07-12 06:02:07 EDT
> 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.)
Comment 13 Laine Stump 2012-07-12 09:38:44 EDT
Whatever is done for this problem, note that the same "duplication" occurs in the network xml, e.g.:

   <forward dev='em4' mode='private'>
    <interface dev='em4'/>
  </forward>

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.
Comment 14 Cole Robinson 2016-03-22 07:35:40 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.