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