Dan Kenigsberg has posted comments on this change. Change subject: udev: Race fix- load and trigger dev rule(#891300) ......................................................................
Patch Set 3: I would prefer that you didn't submit this (3 inline comments) .................................................... File vdsm/supervdsmServer.py Line 329: Line 330: def __udevVersion(self): Line 331: cmd = [EXT_UDEVADM, '--version'] Line 332: rc, out, err = misc.execCmd(cmd, sudo=False) Line 333: global version please try to avoid global variables. and certainly not with such a generic name. please take a look at utils.memoize - I think that all you need is to decorate __udevVersion (after returning a value). Line 334: version = rc Line 335: Line 336: Line 337: def __isEnterprise(self): Line 333: global version Line 334: version = rc Line 335: Line 336: Line 337: def __isEnterprise(self): the name is wrong, since on el7 is going to have __isEnterprise == False. Consider __udevHasReload or something in that spirit. Line 338: if version is None: Line 339: __udevVersion() Line 340: return version < 181 Line 341: Line 336: Line 337: def __isEnterprise(self): Line 338: if version is None: Line 339: __udevVersion() Line 340: return version < 181 str and int comparison Line 341: Line 342: Line 343: def __pokeParent(parentPid, address, log): Line 344: try: -- To view, visit http://gerrit.ovirt.org/10758 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If3b2008a3d9df2dcaf54190721c2dd9764338627 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vered Volansky <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Lee Yarwood <[email protected]> Gerrit-Reviewer: Royce Lv <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
