Change in vdsm[master]: clientIF: prepareVolumePath payload cleanup
fromani at redhat.com
fromani at redhat.com
Mon Jan 13 13:15:22 UTC 2014
Francesco Romani has posted comments on this change.
Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.ovirt.org/#/c/22928/5/vdsm/clientIF.py
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, payload):
> It would be nice to put private methods after the public method they serve.
Done
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 251: raise vm.VolumeError("Missing 'file' attribute in 'payload'")
Line 252:
Line 253: mkFsNames = {'cdrom': 'mkIsoFs', 'floppy': 'mkFloppyFs'}
Line 254: if device not in mkFsNames:
Line 255: raise vm.VolumeError("Unsupported 'device': %s" % device)
> An empty line after we raise makes the flow more clear - see raise in line
Done
Line 256: try:
Line 257: mkFsFunction = getattr(supervdsm.getProxy(),
Line 258: mkFsNames[device])
Line 259: except RuntimeError:
Line 257: mkFsFunction = getattr(supervdsm.getProxy(),
Line 258: mkFsNames[device])
Line 259: except RuntimeError:
Line 260: raise vm.VolumeError("Supervdsm call failed for %s in "
Line 261: "drive: %s" % (device, drive))
> Same
Done
Line 262: return mkFsFunction(vmId, payload['file'], payload.get('volId'))
Line 263:
Line 264: def prepareVolumePath(self, drive, vmId=None):
Line 265: if type(drive) is dict:
Line 308: elif (params.get('path', '') == '' and
Line 309: # line above can be removed in future, when < 3.3 engine
Line 310: # is not supported
Line 311: drive.get('path', '') == ''):
Line 312: volPath = ''
> Do you mean if path is empty or missing on both params and drive, set volPa
Yes. This is a nasty case we need to support due to strange combination of parameters and multiple engine support. It's one of the dark corners I mentioned;
Line 313: else:
Line 314: volPath = drive.get('path', '')
Line 315:
Line 316: elif "path" in drive:
Line 310: # is not supported
Line 311: drive.get('path', '') == ''):
Line 312: volPath = ''
Line 313: else:
Line 314: volPath = drive.get('path', '')
> It would be nice to get a nice comment about each condition, like the one a
Good point. I think the code becomes more robust and cleaner if I just move above this line, and we use this value as default.
Line 315:
Line 316: elif "path" in drive:
Line 317: volPath = drive['path']
Line 318:
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list