Martin Polednik has posted comments on this change. Change subject: hostdev: report additional information in 'scsi' device ......................................................................
Patch Set 3: (5 comments) https://gerrit.ovirt.org/#/c/56038/3//COMMIT_MSG Commit Message: PS3, Line 14: Another addition is udev_path, that we can require to fully construct : the device in future. > This means it is nice-to-have or do we have a clear need/usecase in mind? Required. the wording makes this unclear. https://gerrit.ovirt.org/#/c/56038/3/lib/vdsm/hostdev.py File lib/vdsm/hostdev.py: PS3, Line 156: device_params = { : key: value for key, value in : list_by_caps(device_cap).items() if : value['params']['parent'] == parent_name}.values()[0] > s/unzip/make less concise, more readable/ Tried splitting the comprehension from lookup. Lost track or ValueError origin in the process though. PS3, Line 171: except (KeyError, ValueError, IndexError): : pass > what could raise those? You handle the same exceptions in find_device_by_pa They are raised (explicitly). Either of these serve flow control to indicate that the device couldn't be found. PS3, Line 185: except > Can you be more specific here? Should be similar to 171-172. Fixed. https://gerrit.ovirt.org/#/c/56038/3/tests/hostdevTests.py File tests/hostdevTests.py: PS3, Line 430: hostdev.UnsuitableSCSIDevice > why so? Because the format is kinda awkward in itself. Will move to the permutation with explanation. -- To view, visit https://gerrit.ovirt.org/56038 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I789f49c9abca2dff1487acf3e8a04ae1407956f4 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
