Francesco Romani has posted comments on this change. Change subject: Implementation of logind based session locking ......................................................................
Patch Set 3: (3 comments) Since you changed, a few more comments. http://gerrit.ovirt.org/#/c/24628/3/ovirt-guest-agent/LockActiveSession.py File ovirt-guest-agent/LockActiveSession.py: Line 53: if not iface: Line 54: iface = 'org.freedesktop.%s.%s' % (service, name) Line 55: if not name: Line 56: iface = iface[:-1] Line 57: return dbus.Interface(obj, dbus_interface=iface) I like this it looks a good change to me. Line 58: Line 59: Line 60: def GetInterfaceByName(bus, service, name, isSub): Line 61: path = '/org/freedesktop/' + service Line 59: Line 60: def GetInterfaceByName(bus, service, name, isSub): Line 61: path = '/org/freedesktop/' + service Line 62: if isSub: Line 63: path += '/' + name As matter as personal taste, I'd just used an optional prefix like def GetInterfaceByName(bus, service, name=''): path = '/org/freedesktop/' + service if name: path += '/' + name return GetInterface(bus, service, name, path) Or, if it make sense to the interface partially leak to the client, even directly def GetInterfaceByName(bus, service, name=''): path = '/org/freedesktop/' + service + name return GetInterface(bus, service, name, path) And of course call with name='/something'. However, all the above is mostly personal style/taste, feel free to ignore. Line 64: return GetInterface(bus, service, name, path) Line 65: Line 66: Line 67: def GetSession(bus, service, managerIsSub, wrapSession, wrapManager): Line 81: Line 82: def ManagerWrapper(manager): Line 83: def f(): Line 84: return [x[4] for x in manager.ListSessions()] Line 85: manager.GetSessions = f I think f() should be a staticmethod. Or maybe we can extract GetSessions: def GetSessions(manager): try: return manager.GetSessions() except AttributeError: return [x[4] for x in manager.ListSessions()] This way we can also get rid of ManagerWrapper. Assuming (don't know if that's the case) we must use different methods to get sessions, and the method name is different as the code suggests. Line 86: return manager Line 87: Line 88: Line 89: def GetActiveSession(): -- To view, visit http://gerrit.ovirt.org/24628 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iafc4b88d9c508647e1d09407556359fbefbba34c Gerrit-PatchSet: 3 Gerrit-Project: ovirt-guest-agent Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
