Francesco Romani has posted comments on this change. Change subject: caps: report if QEMU supports live snapshots ......................................................................
Patch Set 6: (6 comments) http://gerrit.ovirt.org/#/c/26149/6//COMMIT_MSG Commit Message: Line 9: Depending on QEMU version and configuration, live snapshotting Line 10: may be not supported. In that case, a request sent by engine Line 11: will of course fail. Line 12: Line 13: Libvirt until recently did not exposed any information about > /me's English is bad, but I think that /me is ashamed of his english. Fixed. Mentioned the libvirt version. Line 14: the live snapshot support, so VDSM, and then engine, had no Line 15: option other than assume this was supported. Line 16: Line 17: Recently libvirt gained this functionality: Line 22: disable the relevant fields in the UI. Line 23: Line 24: The detection is fully backward-compatible. If the information Line 25: is missing from the domain capabilities XML, the support is Line 26: announced to be present. > please consider my suggestion of reporting nothing if we know nothing. Yes, I think it is better. "In the face of ambiguity, refuse the temptation to guess.". Will change the code. Line 27: Line 28: Change-Id: I78dd51fc72f1b6d7eadb5c18d3b768f42d8ee32b Line 29: Bug-Url: https://bugzilla.redhat.com/1009100 http://gerrit.ovirt.org/#/c/26149/6/vdsm/caps.py File vdsm/caps.py: Line 163: Line 164: return topology Line 165: Line 166: Line 167: def _findLiveSnapshotSupport(guest): > The function name is a bit vague, I could use a docstring explaining when " Done Line 168: features = guest.getElementsByTagName('features')[0] Line 169: for feature in features.childNodes: Line 170: if feature.nodeName == 'disksnapshot': Line 171: value = feature.getAttribute('default') Line 171: value = feature.getAttribute('default') Line 172: return value.lower() == 'on' Line 173: # libvirt < 1.2.2 does not export this information. Line 174: # we default as support present Line 175: return True > instead of lying here, how about have a tri-state - True, False, and I-have I agree this is much better. Line 176: Line 177: Line 178: @utils.memoized Line 179: def _getLiveSnapshotSupport(arch, capabilities=None): Line 178: @utils.memoized Line 179: def _getLiveSnapshotSupport(arch, capabilities=None): Line 180: if capabilities is None: Line 181: capabilities = _getCapsXMLStr() Line 182: caps = minidom.parseString(capabilities) > hmm, it's the third time we parse this xml; it might make sense to memoize Added to my TODO. Line 183: Line 184: for guestTag in caps.getElementsByTagName('guest'): Line 185: archTag = guestTag.getElementsByTagName('arch')[0] Line 186: if archTag.getAttribute('name') == arch: Line 185: archTag = guestTag.getElementsByTagName('arch')[0] Line 186: if archTag.getAttribute('name') == arch: Line 187: return _findLiveSnapshotSupport(guestTag) Line 188: Line 189: logging.error('guest capabilities not found') > Do we expect libvirt to ever fail to have the <guest> element for our arch, AFAIK is a "this cannot happen" case. Maybe the log here does more harm than good, will remove Line 190: return False Line 191: Line 192: Line 193: @utils.memoized -- To view, visit http://gerrit.ovirt.org/26149 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78dd51fc72f1b6d7eadb5c18d3b768f42d8ee32b Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Leonardo Bianconi <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Vitor de Lima <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
