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 <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [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
