Federico Simoncelli has uploaded a new change for review.
Change subject: remoteFileHandler: improve PYTHONPATH definition ......................................................................
remoteFileHandler: improve PYTHONPATH definition
The recently added import for logUtils in misc.py broke the out of process (remoteFileHandler) subsystem (ImportError: No module named logUtils). Defining PYTHONPATH with relative paths is both unreliable and insecure.
In this patch: * Remove an old sys.path modification in misc.py * Improve the PYTHONPATH definition in remoteFileHandler removing the relative path
Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Signed-off-by: Federico Simoncelli fsimonce@redhat.com --- M vdsm/storage/misc.py M vdsm/storage/remoteFileHandler.py 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/93/9193/1
diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py index bf3ae79..7453b7f 100644 --- a/vdsm/storage/misc.py +++ b/vdsm/storage/misc.py @@ -55,7 +55,6 @@ import fcntl import inspect
-sys.path.append("../") from vdsm import constants import storage_exception as se from vdsm.betterPopen import BetterPopen diff --git a/vdsm/storage/remoteFileHandler.py b/vdsm/storage/remoteFileHandler.py index edf8e68..933dc82 100644 --- a/vdsm/storage/remoteFileHandler.py +++ b/vdsm/storage/remoteFileHandler.py @@ -220,7 +220,8 @@ try: # Some imports in vdsm assume /usr/share/vdsm is in your PYTHONPATH env = os.environ.copy() - env['PYTHONPATH'] = "../:" + env.get('PYTHONPATH', "") + env['PYTHONPATH'] = "%s:%s" % ( + constants.P_VDSM, env.get("PYTHONPATH"])) self.process = BetterPopen([constants.EXT_PYTHON, __file__, str(hisRead), str(hisWrite)], close_fds=False, env=env)
-- To view, visit http://gerrit.ovirt.org/9193 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com
Federico Simoncelli has posted comments on this change.
Change subject: remoteFileHandler: improve PYTHONPATH definition ......................................................................
Patch Set 1: (1 inline comment)
.................................................... File vdsm/storage/remoteFileHandler.py Line 220: try: Line 221: # Some imports in vdsm assume /usr/share/vdsm is in your PYTHONPATH Line 222: env = os.environ.copy() Line 223: env['PYTHONPATH'] = "%s:%s" % ( Line 224: constants.P_VDSM, env.get("PYTHONPATH"])) double ) typo Line 225: self.process = BetterPopen([constants.EXT_PYTHON, __file__, Line 226: str(hisRead), str(hisWrite)], close_fds=False, env=env) Line 227: Line 228: self.proxy = CrabRPCProxy(myRead, myWrite)
-- To view, visit http://gerrit.ovirt.org/9193 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com
Yaniv Bronhaim has posted comments on this change.
Change subject: remoteFileHandler: improve PYTHONPATH definition ......................................................................
Patch Set 1: Fails
compilation error. please fix it and I'll check again
-- To view, visit http://gerrit.ovirt.org/9193 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com
Zhou Zheng Sheng has posted comments on this change.
Change subject: remoteFileHandler: improve PYTHONPATH definition ......................................................................
Patch Set 3: Verified; Looks good to me, but someone else must approve
I submit a similar fix at http://gerrit.ovirt.org/#/c/9183/1 . This patch looks better, so I abandon mine.
I reported a bug at https://bugzilla.redhat.com/show_bug.cgi?id=875678 Before this patch, I can not connect to LOCALFS storage server using vdsClient. After the patch, it works. Furthermore, I apply the path on my storage and VM creation functional tests (http://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:VMT...) , and it runs successfully.
-- To view, visit http://gerrit.ovirt.org/9193 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Shu Ming has posted comments on this change.
Change subject: remoteFileHandler: improve PYTHONPATH definition ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/storage/remoteFileHandler.py Line 220: try: Line 221: # Some imports in vdsm assume /usr/share/vdsm is in your PYTHONPATH Line 222: env = os.environ.copy() Line 223: env['PYTHONPATH'] = "%s:%s" % ( Line 224: constants.P_VDSM, env.get("PYTHONPATH", "")) Can you make sure that if we still need constants.P_VDSM if "../" is added into the sys path in misc.py? I think they are redundant efforts to locate the files in the upper level directory. Line 225: self.process = BetterPopen([constants.EXT_PYTHON, __file__, Line 226: str(hisRead), str(hisWrite)], close_fds=False, env=env) Line 227: Line 228: self.proxy = CrabRPCProxy(myRead, myWrite)
-- To view, visit http://gerrit.ovirt.org/9193 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Zhou Zheng Sheng has posted comments on this change.
Change subject: remoteFileHandler: improve PYTHONPATH definition ......................................................................
Patch Set 3: (1 inline comment)
.................................................... File vdsm/storage/remoteFileHandler.py Line 220: try: Line 221: # Some imports in vdsm assume /usr/share/vdsm is in your PYTHONPATH Line 222: env = os.environ.copy() Line 223: env['PYTHONPATH'] = "%s:%s" % ( Line 224: constants.P_VDSM, env.get("PYTHONPATH", "")) When run as a deamon, the working dir is "/", so "../" points to "/" as well. The new process created by BetterPopen inherits the working dir of the parent process. So constants.P_VDSM is better than "../" and "../" in misc.py looks not very useful. Line 225: self.process = BetterPopen([constants.EXT_PYTHON, __file__, Line 226: str(hisRead), str(hisWrite)], close_fds=False, env=env) Line 227: Line 228: self.proxy = CrabRPCProxy(myRead, myWrite)
-- To view, visit http://gerrit.ovirt.org/9193 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: remoteFileHandler: improve PYTHONPATH definition ......................................................................
Patch Set 3: Looks good to me, approved
Thanks. Mangling sys.path is evil.
-- To view, visit http://gerrit.ovirt.org/9193 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
Dan Kenigsberg has submitted this change and it was merged.
Change subject: remoteFileHandler: improve PYTHONPATH definition ......................................................................
remoteFileHandler: improve PYTHONPATH definition
The recently added import for logUtils in misc.py broke the out of process (remoteFileHandler) subsystem (ImportError: No module named logUtils). Defining PYTHONPATH with relative paths is both unreliable and insecure.
In this patch: * Remove an old sys.path modification in misc.py * Improve the PYTHONPATH definition in remoteFileHandler removing the relative path
Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Signed-off-by: Federico Simoncelli fsimonce@redhat.com --- M vdsm/storage/misc.py M vdsm/storage/remoteFileHandler.py 2 files changed, 2 insertions(+), 2 deletions(-)
Approvals: Dan Kenigsberg: Looks good to me, approved Zhou Zheng Sheng: Verified; Looks good to me, but someone else must approve
Objections: Shu Ming: I would prefer that you didn't submit this
-- To view, visit http://gerrit.ovirt.org/9193 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I29dd748bcc9658a98ad86313921bfd94c96b83a6 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yaniv Bronhaim ybronhei@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com
vdsm-patches@lists.fedorahosted.org