Change in vdsm[master]: Sometimes the FD number can be reused in the child process

Saggi Mizrahi has uploaded a new change for review.

Change subject: Sometimes the FD number can be reused in the child process
......................................................................

Sometimes the FD number can be reused in the child process

This makes sure that the fd is the file that we expected to be closed child
process

Change-Id: I7044936fba8923297c76d9a2123215ec2b793548
Signed-off-by: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
---
M tests/betterPopenTests.py
1 file changed, 7 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/91/9591/1

diff --git a/tests/betterPopenTests.py b/tests/betterPopenTests.py
index 6347460..eb129f4 100644
--- a/tests/betterPopenTests.py
+++ b/tests/betterPopenTests.py
@@ -59,8 +59,9 @@
 
     def testCloseFDs(self):
         fds = os.pipe()
+        rpath = os.path.realpath("/proc/%d/fds/%d" % (os.getpid(), fds[1]))
         try:
-            self._subTest("fds", [str(fds[1])], close_fds=True)
+            self._subTest("fds", [str(fds[1]), rpath], close_fds=True)
         finally:
             os.close(fds[0])
             os.close(fds[1])
@@ -154,11 +155,11 @@
 if __name__ == "__main__":
     cmd = sys.argv[1]
     if cmd == "fds":
-        try:
-            os.close(int(sys.argv[2]))
-            print "False"
-        except:
-            print "True"
+        fdpath = "/proc/%d/fds/%d" % (os.getpid(), int(sys.argv[2]))
+        if not os.path.exists(fdpath):
+            print True
+        else:
+            print (open(fdpath).read().strip() == sys.argv[3])
 
     elif cmd == "nofds":
         try:


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7044936fba8923297c76d9a2123215ec2b793548
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Yaniv Bronhaim has posted comments on this change.

Change subject: Sometimes the FD number can be reused in the child process
......................................................................


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7044936fba8923297c76d9a2123215ec2b793548
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

Itamar Heim has posted comments on this change.

Change subject: Sometimes the FD number can be reused in the child process
......................................................................


Patch Set 3:

ping

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7044936fba8923297c76d9a2123215ec2b793548
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Sometimes the FD number can be reused in the child process
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File tests/betterPopenTests.py
Line 158:         fdpath = "/proc/%d/fd/%d" % (os.getpid(), int(sys.argv[2]))
Line 159:         if not os.path.exists(fdpath):
Line 160:             print True
Line 161:         else:
Line 162:             print (open(fdpath).read().strip() == sys.argv[3])
I still do not follow. Why are you reading from fdpath, and why do you expect to find
anything there?

why cannot you check if /proc/$$/fd/ has only 0, 1, and 2?
Line 163: 
Line 164:     elif cmd == "nofds":
Line 165:         try:
Line 166:             os.close(int(sys.argv[2]))


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7044936fba8923297c76d9a2123215ec2b793548
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Sometimes the FD number can be reused in the child process
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

sorry, I do not understand the bug in the test or its fix. would you (or someone else)
please explain it better?

....................................................
File tests/betterPopenTests.py
Line 58:         self.assertEquals(p.stdout.read().strip(), "True")
Line 59: 
Line 60:     def testCloseFDs(self):
Line 61:         fds = os.pipe()
Line 62:         rpath = os.path.realpath("/proc/%d/fds/%d" % (os.getpid(),
fds[1]))
on my system it's called /proc/$$/fd/255
(notice the missing s)
Line 63:         try:
Line 64:             self._subTest("fds", [str(fds[1]), rpath], close_fds=True)
Line 65:         finally:
Line 66:             os.close(fds[0])


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7044936fba8923297c76d9a2123215ec2b793548
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has abandoned this change.

Change subject: Sometimes the FD number can be reused in the child process
......................................................................


Abandoned

betterPopen forked out of Vdsm.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I7044936fba8923297c76d9a2123215ec2b793548
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;

Itamar Heim has posted comments on this change.

Change subject: Sometimes the FD number can be reused in the child process
......................................................................


Patch Set 3:

ping

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7044936fba8923297c76d9a2123215ec2b793548
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No