Dan Kenigsberg has posted comments on this change. Change subject: service: make service compatible with systemctl reload rc ......................................................................
Patch Set 4: (1 comment) http://gerrit.ovirt.org/#/c/36491/4/lib/vdsm/tool/service.py File lib/vdsm/tool/service.py: Line 297: rc, out, err = _execSysvEnv(cmd) Line 298: status = service_status(srvName, False) Line 299: if (rc == 0) and (status != 0): Line 300: rc = 1 Line 301: err = 'reload failed because service was not running' > I do not understand what would we benefit from changing rc to 1, it really You are right about the positive flow. My suggestion was wrong. The tiny benefit is that you make it impossible for your users to tell the reason of the failure, just like systemd currently does. I don't see this as a big deal, since systemd could decide tomorrow to make this discernible. More important is to make the follow-up patch handle the excetion instead of checking the non-existing return code. Line 302: return (rc, out, err) Line 303: Line 304: @_sysvNative Line 305: def _serviceIsManaged(srvName): -- To view, visit http://gerrit.ovirt.org/36491 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I878fb898204f7a8a564941b43b12c024ef208765 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[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
