Peter V. Saveliev has posted comments on this change. Change subject: RAM snapshots feature ......................................................................
Patch Set 1: (1 inline comment) .................................................... File vdsm/BindingXMLRPC.py Line 279: def vmUpdateDevice(self, vmId, params): Line 280: vm = API.VM(vmId) Line 281: return vm.vmUpdateDevice(params) Line 282: Line 283: def vmSnapshot(self, vmId, snapDrives, snapMemory=''): I thinks that it is not clear from the code, that the engine uses empty strings for snapshots w/o memory. If someone will change snapMemory default to «None» in the future, the call will continue to work, but occasionally we can fail with the logic change: it is enough to start to use type check to get into error with cluster xor mem-less snapshots. And the cause of the fault will be not clear. So ok, let us use an empty string by default, but please, write some detailed comment in the code about why there is an empty string (not «None» or «False»), and how behave all three cases you described above. I'm sure, it can be important in the future. Line 284: vm = API.VM(vmId) Line 285: return vm.snapshot(snapDrives, snapMemory) Line 286: Line 287: def vmMerge(self, vmId, mergeDrives): -- To view, visit http://gerrit.ovirt.org/15072 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I62401940afb0228cbd9dd3611b6ed8e0ff67c82c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Eduardo <ewars...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Peter V. Saveliev <p...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
