Change in vdsm[master]: vdsClient: file based mechanism to provide password

Alon Bar-Lev alonbl at redhat.com
Fri Feb 21 12:03:50 UTC 2014


Alon Bar-Lev has posted comments on this change.

Change subject: vdsClient: file based mechanism to provide password
......................................................................


Patch Set 5:

(10 comments)

not sure why it is complex.

http://gerrit.ovirt.org/#/c/24733/5/client/vdsClient.py
File client/vdsClient.py:

Line 129: 
Line 130: def checkKeyAndGetPassword(key, value):
Line 131:     if 'auth' == key:
Line 132:         return ('password', getPassword(value))
Line 133:     return ('password', value)
why do you need this function? why do you fake password?
Line 134: 
Line 135: 
Line 136: def checkArgAndGetPassword(arg):
Line 137:     if not arg.startswith('auth'):


Line 135: 
Line 136: def checkArgAndGetPassword(arg):
Line 137:     if not arg.startswith('auth'):
Line 138:         return arg
Line 139:     return getPassword(arg[5:])
why do you have hard coded 5?
Line 140: 
Line 141: 
Line 142: def getPassword(location):
Line 143:     (method, value) = location.split(':')


Line 138:         return arg
Line 139:     return getPassword(arg[5:])
Line 140: 
Line 141: 
Line 142: def getPassword(location):
add prefix='' as parameter, remove prefix before split.
Line 143:     (method, value) = location.split(':')
Line 144:     if 'file' == method:
Line 145:         try:
Line 146:             password_line = None


Line 139:     return getPassword(arg[5:])
Line 140: 
Line 141: 
Line 142: def getPassword(location):
Line 143:     (method, value) = location.split(':')
split(':', 1)
Line 144:     if 'file' == method:
Line 145:         try:
Line 146:             password_line = None
Line 147:             with open(value) as f:


Line 147:             with open(value) as f:
Line 148:                 password_line = f.readline()
Line 149:             return password_line
Line 150:         except IOError:
Line 151:             return value
why do you return file name if you cannot open the file?!?! let the IOException be thrown, it is an error.
Line 152:     elif 'env' == method:
Line 153:         try:
Line 154:             return os.environ[value]
Line 155:         except KeyError:


Line 152:     elif 'env' == method:
Line 153:         try:
Line 154:             return os.environ[value]
Line 155:         except KeyError:
Line 156:             return value
what?!?! if no environment it should either error or empty, sure not the environment key name.
Line 157:     elif 'pass' == method:
Line 158:         return value
Line 159:     else:
Line 160:         raise Exception("Unknown method")


Line 490: 
Line 491:     def desktopLogin(self, args):
Line 492:         vmId, domain, user, password = tuple(args)
Line 493:         if args[-1].startswith('auth'):
Line 494:             password = checkArgAndGetPassword(password)
so you have both auth here and in function, how does it make sense?
Line 495:         response = self.s.desktopLogin(vmId, domain, user, password)
Line 496:         print response['status']['message']
Line 497:         sys.exit(response['status']['code'])
Line 498: 


Line 728:         else:
Line 729:             username = args[1]
Line 730:             password = args[2]
Line 731:             if args[2].startswith('auth'):
Line 732:                 password = checkArgAndGetPassword(password)
password = getPassword(password, 'auth=')
Line 733: 
Line 734:         con = dict(id="", connection=ip, port=port, iqn="", portal="",
Line 735:                    user=username, password=password)
Line 736: 


Line 757:         conList = []
Line 758:         con = {}
Line 759:         for item in params:
Line 760:             key, value = item.split('=')
Line 761:             key, value = checkKeyAndGetPassword(key, value)
should be:

 if key == 'auth':
     key = 'password'  # I do not like it, but probably you have some issues
     value = getPassword(value)

as simple as that.
Line 762:             con[key] = value
Line 763:         conList.append(con)
Line 764:         res = self.s.connectStorageServer(serverType, spUUID, conList)
Line 765:         if res['status']['code']:


Line 773:         conList = []
Line 774:         con = {}
Line 775:         for item in params:
Line 776:             key, value = item.split('=')
Line 777:             key, value = checkKeyAndGetPassword(key, value)
same here
Line 778:             con[key] = value
Line 779:         conList.append(con)
Line 780:         res = self.s.validateStorageServerConnection(serverType,
Line 781:                                                      spUUID, conList)


-- 
To view, visit http://gerrit.ovirt.org/24733
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I363a16e6a7872ca05e19d5f520bdba90fb492374
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yedidyah Bar David <didi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list