Nir Soffer has posted comments on this change. Change subject: vdscli: no need to generate using autoconf ......................................................................
Patch Set 2: Code-Review+1 (4 comments) Lot can be improved in later patches. http://gerrit.ovirt.org/#/c/28676/2/lib/vdsm/vdscli.py File lib/vdsm/vdscli.py: Line 29: Line 30: d_useSSL = False Line 31: d_tsPath = '/etc/pki/vdsm' Line 32: d_addr = '0' Line 33: d_port = '54321' These should be UPPERCASE and I would love to see the d_ prefix killed. Line 34: Line 35: Line 36: def wrap_transport(transport): Line 37: old_parse_response = transport.parse_response Line 74: pass Line 75: except: Line 76: pass Line 77: Line 78: > I'll try to think of a means to rewrite this in a less convoluted way. I would simply initialize d_stuff from the config when the module is imported, from the config. Line 79: __guessDefaults() Line 80: Line 81: Line 82: def cannonizeHostPort(hostPort=None, port=d_port): Line 93: Line 94: return addr + ':' + port Line 95: Line 96: Line 97: def connect(hostPort=None, useSSL=None, tsPath=None, > Having a method with uncertain defaults (coming from an implicit config fil We can use hostPort=ADDRESS, where ADDRESS is initialized from config.py on import time. Line 98: TransportClass=SecureXMLRPCServer.VerifyingSafeTransport): Line 99: hostPort = cannonizeHostPort(hostPort) Line 100: if useSSL is None: Line 101: useSSL = d_useSSL Line 117: else: Line 118: transport = wrap_transport(SingleRequestTransport()) Line 119: server = xmlrpclib.Server('http://%s' % hostPort, transport) Line 120: return server Line 121: > connect() with no args is used extensively (also outside the project) to me connect() is evil, but since it is used everywhere, lets keep it. Line 122: if __name__ == '__main__': Line 123: print 'connecting to %s:%s ssl %s ts %s' % (d_addr, d_port, Line 124: d_useSSL, d_tsPath) Line 125: server = connect() -- To view, visit http://gerrit.ovirt.org/28676 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I13059d101068cf6fa292baae4fb93ccf0b30db45 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: mooli tayer <[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
