Nir Soffer has posted comments on this change.
Change subject: open: Change file() to open()
......................................................................
Patch Set 4: Code-Review-1
(1 comment)
The patch contain unrelated and wrong change. This is not a replacement of
"file" with "open".
http://gerrit.ovirt.org/#/c/26776/4/lib/vdsm/libvirtconnection.py
File lib/vdsm/libvirtconnection.py:
Line 130: for cred in credentials:
Line 131: if cred[0] == libvirt.VIR_CRED_AUTHNAME:
Line 132: cred[4] = constants.SASL_USERNAME
Line 133: elif cred[0] == libvirt.VIR_CRED_PASSPHRASE:
Line 134: with open(constants.P_VDSM_LIBVIRT_PASSWD) as fileStream:
fileStream? We don't use such term in Python. The typical name for this is
"f", which is just fine for a scope of one line. If you want nicer name, call it
"file". If you think this is not clear enough, call it "passwd_file".
The previous code open the file and read the first line once. Here you open the file and
read it for each item in the credentials list. This is not what the commit message
promise.
The correct fix is to replace:
passwd = file(constants.P_VDSM_LIBVIRT_PASSWD).readline().rstrip("\n")
With:
passwd = open(constants.P_VDSM_LIBVIRT_PASSWD).readline().rstrip("\n")
Using with to ensure that the file is always closed is good change but not related to the
purpose of this patch. Please make it easy for your reviewers and focus on one logical
change.
Line 135: cred[4] = fileStream.readline().rstrip("\n")
Line 136: return 0
Line 137:
Line 138: auth = [[libvirt.VIR_CRED_AUTHNAME, libvirt.VIR_CRED_PASSPHRASE],
--
To view, visit
http://gerrit.ovirt.org/26776
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I80007044be1c648ebb668704731eae8b83366025
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dima Kuznetsov <dkuznets(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes