Nir Soffer has posted comments on this change. Change subject: clientIF: prepareVolumePath payload cleanup ......................................................................
Patch Set 4: Code-Review-1 (1 comment) Some parts became worse then the original code in latest patches. http://gerrit.ovirt.org/#/c/24636/4/vdsm/clientIF.py File vdsm/clientIF.py: Line 337: return supervdsm.getProxy().mkIsoFs(vmId, payload['file'], Line 338: payload.get('volId')) Line 339: elif device == 'floppy': Line 340: return supervdsm.getProxy().mkFloppyFs(vmId, payload['file'], Line 341: payload.get('volId')) The only difference in these calls is the function name but you repeat everything for no good reason. The old code (lines 299-310) was better. Cleaning up the old code, we can do this: funcs = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'} try: func = getattr(supervdsm.getProxy(), funcs[device]) except AttributeError: raise vm.VolumeError... return func(vmId, payload['file'], payload.get('volId')) In another patch, we can improve this so we don't call the proxy to report unsupported device: funcs = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'} if device not in funcs: raise vm.VolumeError... func = getattr(supervdsm.getProxy(), funcs[device]) return func(vmId, payload['file'], payload.get('volId')) The second version will not raise KeyError on bad input, and AttributeError from the proxy is impossible since it implements both functions. But this changes the behavior and does not belong in this refactoring patch. Line 342: else: Line 343: raise vm.VolumeError("Unsupported 'device': %s" % device) Line 344: Line 345: def teardownVolumePath(self, drive): -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[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
