Nir Soffer has posted comments on this change. Change subject: vdsm: prepareVolumePath payload detection cleanup ......................................................................
Patch Set 4: (10 comments) The new method is clean and not tricky, and the tests are simple and clean. I looked only very briefly at the tests and I'll do another pass later. See the comments for possible cleanups and simplification. .................................................... File tests/clientifTests.py Line 24: from vm import VolumeError Line 25: import clientIF Line 26: Line 27: Line 28: class supervdsmMock: This is not a mock but a fake - supervdsmFake. See http://martinfowler.com/articles/mocksArentStubs.html Line 29: def __init__(self, cifMock=None): Line 30: self.cifMock = cifMock Line 31: Line 32: def getProxy(self): Line 40: setattr(self.cifMock, 'builtFloppy', True) Line 41: return 'floppy' Line 42: Line 43: Line 44: class clientIFMock(clientIF.clientIF): Again, this is a fake, not a mock. Line 45: def __init__(self): Line 46: self.irs = None # just to make sure nothing ever happens Line 47: self.log = logging.getLogger('clientIFTest') Line 48: self.builtFloppy = False Line 48: self.builtFloppy = False Line 49: self.builtCdrom = False Line 50: Line 51: Line 52: class clientIFTests(TestCaseBase): Whitspace between class and next method make it more readable. Line 53: def setUp(self): Line 54: self.cif = clientIFMock() Line 55: # this is a necessary evil. Line 56: clientIF.supervdsm = supervdsmMock(self.cif) Line 51: Line 52: class clientIFTests(TestCaseBase): Line 53: def setUp(self): Line 54: self.cif = clientIFMock() Line 55: # this is a necessary evil. Nothing evil here :-) Line 56: clientIF.supervdsm = supervdsmMock(self.cif) Line 57: Line 58: def testThisModuleAsDrive(self): Line 59: volPath = self.cif.prepareVolumePath(__file__) .................................................... File vdsm/clientIF.py Line 240: except blkid.BlockIdException: Line 241: self.log.info('Error finding path for device', exc_info=True) Line 242: raise vm.VolumeError(uuid) Line 243: Line 244: def _prepareVolumeFromPayload(self, vmId, drive, device, params): Lets accept payload instead of params, since the caller already know it exists. Line 245: ''' Line 246: vmPayload is a key in params Line 247: 'vmPayload': {'volId': 'volume id', # volId is optional Line 248: 'file': {'filename': 'content', ...}} Line 246: vmPayload is a key in params Line 247: 'vmPayload': {'volId': 'volume id', # volId is optional Line 248: 'file': {'filename': 'content', ...}} Line 249: ''' Line 250: mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'} It is little ugly that we have to dispatch 'cdrom' and 'floppy' here - it would be much nicer if supervdsmServer was doing this dispatching. Lets first make sure that device is valid - if not, lets raise ValueError, or better a more specific VolumeError: if 'device' not in mkFsNames: raise ValueError(device) # VolumeError? This fix the unhandled KeyError in line 253. Line 251: try: Line 252: mkFsFunction = getattr(supervdsm.getProxy(), Line 253: mkFsNames[device]) Line 254: except AttributeError: Line 249: ''' Line 250: mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'} Line 251: try: Line 252: mkFsFunction = getattr(supervdsm.getProxy(), Line 253: mkFsNames[device]) Can raise KeyError. We don't want to handle it here since this is a rather expensive call to another process, and we dont want to mix KeyError raised from supervdsm.getProxy() with KeyError from mkFsNames[device]. Line 254: except AttributeError: Line 255: raise vm.VolumeError("Unsupported 'device': %s in " Line 256: "drive: %s" % (device, drive)) Line 257: else: Line 252: mkFsFunction = getattr(supervdsm.getProxy(), Line 253: mkFsNames[device]) Line 254: except AttributeError: Line 255: raise vm.VolumeError("Unsupported 'device': %s in " Line 256: "drive: %s" % (device, drive)) Since now we use one of the function names defined by supervdsmServer, checking AttributeError is not needed now - if this code fails, the error is not unsupported device but some bug in supervdsmServer. Check what errors other code raise when trying to get supervdsm proxy method fails. Line 257: else: Line 258: payload = params['vmPayload'] Line 259: files = payload['file'] Line 260: volId = payload.get('volId') Line 253: mkFsNames[device]) Line 254: except AttributeError: Line 255: raise vm.VolumeError("Unsupported 'device': %s in " Line 256: "drive: %s" % (device, drive)) Line 257: else: The else is not needed, as we just raised. This pattern is needed when there are two code paths, that merge after the else. Line 258: payload = params['vmPayload'] Line 259: files = payload['file'] Line 260: volId = payload.get('volId') Line 261: return mkFsFunction(vmId, files, volId) Line 258: payload = params['vmPayload'] Line 259: files = payload['file'] Line 260: volId = payload.get('volId') Line 261: return mkFsFunction(vmId, files, volId) Line 262: The temporary variables are not needed (assuming we got a payload parameter): return mkFsFunction(vmId, payload['file'], payload.get('volId')) I assume from your code that 'file' is required parameter and you want to raise a KeyError here if missing, and 'volId' is optional parameter, and you want to silently ignore it if missing. Is this right? Maybe payload should be validated at the top before doing anything else. Line 263: def prepareVolumePath(self, drive, vmId=None): Line 264: if type(drive) is dict: Line 265: device = drive['device'] Line 266: # PDIV drive format -- To view, visit http://gerrit.ovirt.org/22928 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3a630d74ec0910c669e0326ad343c5dbea25357e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@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: Michal Skrivanek <michal.skriva...@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
