Francesco Romani has posted comments on this change. Change subject: clientIF: prepareVolumePath payload cleanup ......................................................................
Patch Set 4: (6 comments) 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 unnee Nice one. Will fix. 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. Will remove. 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 thes Will reduce the comment and try to add more value to the comment 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 294: # better explicit than implicit, thus leave path == '' Line 295: # for empty cdrom and floppy drives. 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 > This comment was above the if in the old code, which make it more clear. Will fix. Line 299: drive.get('path', '') == ''): Line 300: volPath = '' Line 301: else: Line 302: # we are fine with the initial value of volPath 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 dr Done Line 304: Line 305: elif "path" in drive: Line 306: volPath = drive['path'] Line 307: Line 321: Line 322: self.log.info("prepared volume path: %s", volPath) Line 323: return volPath Line 324: Line 325: def _makePayloadVolume(self, vmId, device, payload): > This name is worse than the previous name: _prepareVolumeFromPayload. Are y Fully agree with the name. Will revert. however pep8 1.4.6 doesn't like the indentation: ./vdsm/clientIF.py:290:25: E128 continuation line under-indented for visual indent will (re)introduce a temporary for this sole reason. Line 326: """ Line 327: param vmId: Line 328: VM UUID or None Line 329: param device: -- 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
