Change in vdsm[master]: open: Change file() to open()

nsoffer at redhat.com nsoffer at redhat.com
Mon Jun 23 22:57:19 UTC 2014


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 at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei 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