Dan Kenigsberg has posted comments on this change. Change subject: clientIF: prepareVolumePath payload cleanup ......................................................................
Patch Set 2: Code-Review-1 (2 comments) http://gerrit.ovirt.org/#/c/24636/2/vdsm/clientIF.py File vdsm/clientIF.py: Line 297: elif (params.get('path', '') == '' and Line 298: # line above can be removed in future, when < 3.3 engine Line 299: # is not supported Line 300: drive.get('path', '') == ''): Line 301: volPath = '' do we intentionally fall out of the inner condition silently? if so, add else: pass if not, add else: raise Line 302: Line 303: elif "path" in drive: Line 304: volPath = drive['path'] Line 305: Line 338: # This is so generic because supervdsm.ProxyCaller wraps a Line 339: # multiprocessing' RemoteError into a RuntimeError. Line 340: raise vm.VolumeError("Supervdsm call failed for %s in " Line 341: "drive: %s" % (device, drive)) Line 342: I find the following implementation of the function much simpler, functionally equivalent, and thus preferable. I see no reason to catch and translate supervdsm-induced RuntimeError. The modified exception gives me no further information, and only hides the (improbable, I know) case of a RuntimeError dissipating from within mkIsoFs. if device == 'cdrom': return supervdsm.getProxy().mkIsoFs(vmId, payload['file'], payload.get('volId') elif device == 'floppy': return supervdsm.getProxy().mkFloppyFs(vmId, payload['file'], payload.get('volId') else: raise vm.VolumeError("Unsupported 'device': %s" % device) I'd understand it if you'd like to postpone this change to a follow-up patch, but then, the introduction of "except RuntimeError" could wait, too. Line 343: return mkFsFunction(vmId, payload['file'], payload.get('volId')) Line 344: Line 345: def teardownVolumePath(self, drive): Line 346: res = {'status': doneCode} -- To view, visit http://gerrit.ovirt.org/24636 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I058206b7506ddbb5ec087c9ea0963a10ed57affb Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Eduardo <ewars...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpole...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
