Change in vdsm[master]: clientIF: prepareVolumePath payload cleanup

nsoffer at redhat.com nsoffer at redhat.com
Wed Mar 19 16:36:38 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: clientIF: prepareVolumePath payload cleanup
......................................................................


Patch Set 4:

(4 comments)

Puting some code in the final else makes the code more clear and allow dropping some comments. Also some other comments are not helping.

http://gerrit.ovirt.org/#/c/24636/4/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 281:             # cdrom and floppy drives
Line 282:             elif (device in ('cdrom', 'floppy') and 'specParams' in drive):
Line 283:                 # this is the easy case, and a sane default.
Line 284:                 # Now things can get messy due to strange combinations.
Line 285:                 volPath = drive.get('path', '')
If the line above move to the final else, the 2 comments above it are unneeded.
Line 286:                 # vmPayload is a special case for those devices
Line 287:                 # which needs to be handled first.
Line 288:                 # caution: this code is tricky. See for example:
Line 289:                 # https://bugzilla.redhat.com/show_bug.cgi?id=1047356


Line 284:                 # Now things can get messy due to strange combinations.
Line 285:                 volPath = drive.get('path', '')
Line 286:                 # vmPayload is a special case for those devices
Line 287:                 # which needs to be handled first.
Line 288:                 # caution: this code is tricky. See for example:
This warning does not help - I don't see anything tricky around.
Line 289:                 # https://bugzilla.redhat.com/show_bug.cgi?id=1047356
Line 290:                 params = drive['specParams']
Line 291:                 if 'vmPayload' in params:
Line 292:                     volPath = self._makePayloadVolume(vmId, device,


Line 291:                 if 'vmPayload' in params:
Line 292:                     volPath = self._makePayloadVolume(vmId, device,
Line 293:                                                       params['vmPayload'])
Line 294:                 # better explicit than implicit, thus leave path == ''
Line 295:                 # for empty cdrom and floppy drives.
I don't see any value in this comment. The code is quite clear and all these comments are just noise making it hard to read.

It is clear that the code is trying to treat {"path": ""} and {} as equal cases. The comment can add value if it explain why these case should be considered equal.

More value can be added explaining in which case we get params['path'] and on which cases we get drive['path'].
Line 296:                 elif (params.get('path', '') == '' and
Line 297:                       # line above can be removed in future, when < 3.3 engine
Line 298:                       # is not supported
Line 299:                       drive.get('path', '') == ''):


Line 299:                       drive.get('path', '') == ''):
Line 300:                     volPath = ''
Line 301:                 else:
Line 302:                     # we are fine with the initial value of volPath
Line 303:                     pass
If we move the initial volPath here, the code would be more clear and we drop two comments.
    else:
        volPath = drive.get('path', '')
Line 304: 
Line 305:             elif "path" in drive:
Line 306:                 volPath = drive['path']
Line 307: 


-- 
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 <fromani at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap 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: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list