Ayal Baron has posted comments on this change. Change subject: BZ#784931 - Fixing raise condition from deactivateSD(). ......................................................................
Patch Set 2: I would prefer that you didn't submit this (7 inline comments) .................................................... Commit Message Line 7: BZ#784931 - Fixing raise condition from deactivateSD(). s/raise/race/ s/from/in/ .................................................... File vdsm/storage/mount.py Line 46: def _runcmd(self, cmd, timeout): You changed this into a module function but did not remove self, yet invoking methods do not pass self which leads me to think that this code was never tested as it couldn't not work. Line 48: p = misc.execCmd(cmd, sudo=not isRoot, sync=False) if this requires sudo it probably means it should be run from superVdsm I also don't understand why we need a new runcmd command (and yes, I know you just moved this function around) Line 60: raise MountError(rc, ";".join((out, err))) this function gets a generic cmd so MountError has no business here (and yes, I know you didn't change this, but then again, you probably shouldn't have moved it either) Line 149: m = getMountFromTarget(target) the assignment is redundant, just return getMountFromTarget Line 152: return None should return False, not None. Line 154: else: else is redundant -- To view, visit http://gerrit.ovirt.org/5677 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If06f9cc8da81996590c366de9ad9ea2786a5d3ea Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Igor Lvovsky <[email protected]> Gerrit-Reviewer: Shu Ming <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://fedorahosted.org/mailman/listinfo/vdsm-patches
