Francesco Romani has posted comments on this change. Change subject: supervdsm_udev: generalize udev rule creation ......................................................................
Patch Set 1: Code-Review-1 (3 comments) the concept is fine. Some improvements inside, -1 for visibility. I strongly recommend to way for other reviews before to follow my suggestions, they are discussion points, not requirements. https://gerrit.ovirt.org/#/c/55191/1/vdsm/supervdsm_api/udev.py File vdsm/supervdsm_api/udev.py: PS1, Line 96: : : : please keep this comment - or explain why you dropped it! PS1, Line 155: rule_file file_path? "rule_file, rule" reads awkward :\ PS1, Line 167: _log.debug("Creating rule %s: %r", rule_file, rule) I know that you just moved existing code, but -perhaps in a later patch?- I'd move this log outside the `with' block. -- To view, visit https://gerrit.ovirt.org/55191 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic5bcf6692307595c4a56dae960fb8866d24b809c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
