Nir Soffer has posted comments on this change. Change subject: virt: set correct permissions for hwrng device ......................................................................
Patch Set 9: Code-Review-1 (2 comments) https://gerrit.ovirt.org/#/c/54806/9/vdsm/supervdsm_api/udev.py File vdsm/supervdsm_api/udev.py: Line 62: rule_file = _UDEV_RULE_FILE_NAME % ('hwrng', vmId) Line 63: rule = 'RUN+="%s %s:%s %s"\n' % ( Line 64: EXT_CHOWN, QEMU_PROCESS_USER, QEMU_PROCESS_GROUP, _HWRNG_PATH) Line 65: _mkrule(rule_file, rule) Line 66: _udevTrigger(name_matches=(_HWRNG_PATH,)) This pattern is repeated everywhere - after writing a rule, we always call trigger - right? Our helper for adding a rule should handle this, so it is hard to write new code that does not do the right thing. Line 67: Line 68: Line 69: @expose Line 70: def rmAppropriateHwrngDevice(vmId): Line 69: @expose Line 70: def rmAppropriateHwrngDevice(vmId): Line 71: rule_file = _UDEV_RULE_FILE_NAME % ('hwrng', vmId) Line 72: _log.debug("Removing rule %s", rule_file) Line 73: utils.rmFile(rule_file) Why not use the helper you added in the previous patches? This pattern, logging about removing a rule and then removing it is common and can be extracted to a helper. utils.rmFile cannot be used as it always log a warning about missing files. These false warnings trigger customers bugs about these warnings. It also log exceptions and then re-raise them, causing the same error to appear twice in the logs, very bad pattern. I would not accept new code using rmFile. Line 74: Line 75: # Check that there are no other hwrng rules in place Line 76: if not _rgrep('hwrng-.*'): Line 77: _chown(_HWRNG_PATH, 'root', 'root') -- To view, visit https://gerrit.ovirt.org/54806 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id958a291e5a15813309928ba3d8c67828273b6c6 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Milan Zamazal <[email protected]> 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
