Hello I downloaded Python's 2.5.1 (final) bz2 source archive the other day to try to compile it in MinGW. I have noted down the following observations that might be of interest. 1. The bz2 archive ships with \Modules\collectionsmodule.c instead of the \Modules\_collectionsmodule.c used in the 2.5 SVN branch. In fact the collectionsmodule.c was removed some time ago. 2. If _collectionsmodule.c is the one to be used in branch and source then it follows that \PC\config.c needs an update. 3. test_1686475 of test_os appears to rely on the existence of "c:\pagefile.sys" like so: def test_1686475(self): # Verify that an open file can be stat'ed try: os.stat(r"c:\pagefile.sys") except WindowsError, e: if e == 2: # file does not exist; cannot run test return self.fail("Could not stat pagefile.sys") But since that file does not appear to be in my C drive and since the Windows error returned is not a numeric, but rather a string of the sort: "[Error 5] Access is denied: 'c:\\pagefile.sys'" then that test fails for me both in the MinGW compiled Python and in the officially distributed one. 4. Also test_1565150 of test_os which reads as follows: # Restrict test to Win32, since there is no guarantee other # systems support centiseconds if sys.platform == 'win32': def test_1565150(self): t1 = 1159195039.25 os.utime(self.fname, (t1, t1)) self.assertEquals(os.stat(self.fname).st_mtime, t1) fails in the MinGW compiled Python with the following message: ====================================================================== FAIL: test_1565150 (test.test_os.StatAttributeTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "G:\projs\py25\python\r25\lib\test\test_os.py", line 241, in test_1565150 self.assertEquals(os.stat(self.fname).st_mtime, t1) AssertionError: 1159195040 != 1159195039.25 If the same test passes in the official CPython on the same machine (and it does), can it then be deduced that this is not a system's issue but a compiler one? Thanks Khalid __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
Khalid A. Bakr schrieb:
1. The bz2 archive ships with \Modules\collectionsmodule.c instead of the \Modules\_collectionsmodule.c used in the 2.5 SVN branch. In fact the collectionsmodule.c was removed some time ago.
Why do you say that? http://svn.python.org/projects/python/branches/release25-maint/Modules/colle... is still present AFAICT.
2. If _collectionsmodule.c is the one to be used in branch and source then it follows that \PC\config.c needs an update.
But it isn't the one to be used.
But since that file does not appear to be in my C drive and since the Windows error returned is not a numeric, but rather a string of the sort: "[Error 5] Access is denied: 'c:\\pagefile.sys'" then that test fails for me both in the MinGW compiled Python and in the officially distributed one.
That's true. Can you come up with a patch? Looking at the error code of the exception should be sufficient; it should be 5 (as the message shows). The exception is *not* a string, but an object.
4. Also test_1565150 of test_os which reads as follows: [...] fails in the MinGW compiled Python with the following message:
====================================================================== FAIL: test_1565150 (test.test_os.StatAttributeTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "G:\projs\py25\python\r25\lib\test\test_os.py", line 241, in test_1565150
self.assertEquals(os.stat(self.fname).st_mtime, t1) AssertionError: 1159195040 != 1159195039.25
That would indicate a bug in the MingW port.
If the same test passes in the official CPython on the same machine (and it does), can it then be deduced that this is not a system's issue but a compiler one?
Likely, neither nor. My guess is that the MingW port, for some reason, decides not to use the Win32 API to perform stat, but the C library. That is incorrect, as the C library will perform truncation of subsecond time stamps. The compiler itself should have no effect (other than defining different compiler recognition macros). Regards, Martin
--- "Martin v. Löwis" <martin@v.loewis.de> wrote:
Khalid A. Bakr schrieb:
1. The bz2 archive ships with \Modules\collectionsmodule.c instead of the \Modules\_collectionsmodule.c used in the 2.5 SVN branch. In fact the collectionsmodule.c was removed some time ago.
Why do you say that?
http://svn.python.org/projects/python/branches/release25-maint/Modules/colle...
is still present AFAICT.
2. If _collectionsmodule.c is the one to be used in branch and source then it follows that \PC\config.c needs an update.
But it isn't the one to be used.
I am sorry. Me repository seems corrupted. Doing a clean check out of the 2.5 branch as I write this.
That's true. Can you come up with a patch? Looking at the error code of the exception should be sufficient; it should be 5 (as the message shows). The exception is *not* a string, but an object.
Okay. It seems I mixed up WindowsError with the exception e in my post; at least it is now known that e is not a number. The patch is short and is as follows: Index: Lib/test/test_os.py =================================================================== --- Lib/test/test_os.py (revision 55014) +++ Lib/test/test_os.py (working copy) @@ -245,7 +245,8 @@ try: os.stat(r"c:\pagefile.sys") except WindowsError, e: - if e == 2: # file does not exist; cannot run test + # file may not exist, or access is denied; cannot run test + if e.winerror == 2 or e.winerror == 5: return self.fail("Could not stat pagefile.sys") Or do I need to submit this through sourceforge?
That would indicate a bug in the MingW port.
If the same test passes in the official CPython on the same machine (and it does), can it then be deduced that this is not a system's issue but a compiler one?
Likely, neither nor. My guess is that the MingW port, for some reason, decides not to use the Win32 API to perform stat, but the C library. That is incorrect, as the C library will perform truncation of subsecond time stamps. The compiler itself should have no effect (other than defining different compiler recognition macros).
Regards, Martin
I will try to check what can be done about this. Regards, Khalid __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
--- "Martin v. Löwis" <martin@v.loewis.de> wrote:
Or do I need to submit this through sourceforge?
Please do. Why are you checking for error 2, though, if the error that occurs is 5?
Regards, Martin
Done. I was just correcting the previous posted check for error 2; I thought it must have been there for a reason I don't know. Anyway, this part is now removed from patch, which is at: http://python.org/sf/1709112 Regards, Khalid __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
On 4/27/07, Khalid A. Bakr <khabkr@yahoo.com> wrote:
Okay. It seems I mixed up WindowsError with the exception e in my post; at least it is now known that e is not a number. The patch is short and is as follows:
Index: Lib/test/test_os.py =================================================================== --- Lib/test/test_os.py (revision 55014) +++ Lib/test/test_os.py (working copy) @@ -245,7 +245,8 @@ try: os.stat(r"c:\pagefile.sys") except WindowsError, e: - if e == 2: # file does not exist; cannot run test + # file may not exist, or access is denied; cannot run test + if e.winerror == 2 or e.winerror == 5: return self.fail("Could not stat pagefile.sys")
I have a patch myself that creates an open file and uses that as the test. My reasoning is that pagefile.sys was chosen as a file that should always exist and be open, so its safe to test against, so we should just be testing against a fixture, instead. It is here, and if someone would reference a SF bug report, I'll attach to it, as well. I should also point out that I got the time errors as well, but with different incorrect results. However, I can't seem to reproduce it this morning, but I can say that I did have the test failing for me on VC yesterday. Index: test_os.py =================================================================== --- test_os.py (revision 54982) +++ test_os.py (working copy) @@ -6,6 +6,7 @@ import unittest import warnings import sys +import tempfile from test import test_support warnings.filterwarnings("ignore", "tempnam", RuntimeWarning, __name__) @@ -241,13 +242,18 @@ self.assertEquals(os.stat(self.fname).st_mtime, t1) def test_1686475(self): + fn = tempfile.mktemp() + openfile = open(fn, 'w') # Verify that an open file can be stat'ed try: - os.stat(r"c:\pagefile.sys") + os.stat(fn) except WindowsError, e: if e == 2: # file does not exist; cannot run test return self.fail("Could not stat pagefile.sys") + finally: + openfile.close() + os.remove(fn) from test import mapping_tests -- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
On 4/28/07, Calvin Spealman <ironfroggy@gmail.com> wrote:
Index: test_os.py =================================================================== --- test_os.py (revision 54982) +++ test_os.py (working copy) @@ -6,6 +6,7 @@ import unittest import warnings import sys +import tempfile from test import test_support
warnings.filterwarnings("ignore", "tempnam", RuntimeWarning, __name__) @@ -241,13 +242,18 @@ self.assertEquals(os.stat(self.fname).st_mtime, t1)
def test_1686475(self): + fn = tempfile.mktemp() + openfile = open(fn, 'w') # Verify that an open file can be stat'ed try: - os.stat(r"c:\pagefile.sys") + os.stat(fn) except WindowsError, e: if e == 2: # file does not exist; cannot run test return self.fail("Could not stat pagefile.sys") + finally: + openfile.close() + os.remove(fn)
from test import mapping_tests
mktemp() is deprecated. You may want to use mkstemp(). There will be no need for explicit open as well as mkstemp() also returns open descriptor. Thanks, Raghu.
On Sat, 28 Apr 2007 09:32:57 -0400, Raghuram Devarakonda <draghuram@gmail.com> wrote:
On 4/28/07, Calvin Spealman <ironfroggy@gmail.com> wrote:
Index: test_os.py =================================================================== --- test_os.py (revision 54982) +++ test_os.py (working copy) @@ -6,6 +6,7 @@ import unittest import warnings import sys +import tempfile from test import test_support
warnings.filterwarnings("ignore", "tempnam", RuntimeWarning, __name__) @@ -241,13 +242,18 @@ self.assertEquals(os.stat(self.fname).st_mtime, t1)
def test_1686475(self): + fn = tempfile.mktemp() + openfile = open(fn, 'w') # Verify that an open file can be stat'ed try: - os.stat(r"c:\pagefile.sys") + os.stat(fn) except WindowsError, e: if e == 2: # file does not exist; cannot run test return self.fail("Could not stat pagefile.sys") + finally: + openfile.close() + os.remove(fn)
from test import mapping_tests
mktemp() is deprecated. You may want to use mkstemp(). There will be no need for explicit open as well as mkstemp() also returns open descriptor.
You still need fdopen() though, since os.stat() won't take a file descriptor. The patch is incomplete though, since it should remove the ENOENT handling and the remaining reference to pagefile.sys. As for mktemp() being deprecated - the docstring warns users away, but actually calling it emits no warning. Sure, using it can lead to insecurities, but there's hardly any worry of that here. If the function were actually deprecated (that is, if calling it emitted a DeprecationWarning), that would be a good reason to avoid calling it, though. Jean-Paul
On 4/28/07, Jean-Paul Calderone <exarkun@divmod.com> wrote:
On Sat, 28 Apr 2007 09:32:57 -0400, Raghuram Devarakonda <draghuram@gmail.com> wrote:
On 4/28/07, Calvin Spealman <ironfroggy@gmail.com> wrote:
Index: test_os.py =================================================================== --- test_os.py (revision 54982) +++ test_os.py (working copy) @@ -6,6 +6,7 @@ import unittest import warnings import sys +import tempfile from test import test_support
warnings.filterwarnings("ignore", "tempnam", RuntimeWarning, __name__) @@ -241,13 +242,18 @@ self.assertEquals(os.stat(self.fname).st_mtime, t1)
def test_1686475(self): + fn = tempfile.mktemp() + openfile = open(fn, 'w') # Verify that an open file can be stat'ed try: - os.stat(r"c:\pagefile.sys") + os.stat(fn) except WindowsError, e: if e == 2: # file does not exist; cannot run test return self.fail("Could not stat pagefile.sys") + finally: + openfile.close() + os.remove(fn)
from test import mapping_tests
mktemp() is deprecated. You may want to use mkstemp(). There will be no need for explicit open as well as mkstemp() also returns open descriptor.
You still need fdopen() though, since os.stat() won't take a file descriptor.
The patch is incomplete though, since it should remove the ENOENT handling and the remaining reference to pagefile.sys.
As for mktemp() being deprecated - the docstring warns users away, but actually calling it emits no warning. Sure, using it can lead to insecurities, but there's hardly any worry of that here. If the function were actually deprecated (that is, if calling it emitted a DeprecationWarning), that would be a good reason to avoid calling it, though.
Jean-Paul
Yes, duh, I changed the pagefile.sys in the message, which was silly to have left. As for os.fdopen(), it is not needed. tempfile.mkstemp() returns both the file descriptor and the path, so we simply ignore the file descriptor and pass the path to os.stat(). I'll report the patch with the message fixed. -- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
I have a patch myself that creates an open file and uses that as the test. My reasoning is that pagefile.sys was chosen as a file that should always exist and be open, so its safe to test against, so we should just be testing against a fixture, instead. It is here, and if someone would reference a SF bug report, I'll attach to it, as well.
There must be more to the problem than just an open file. Please undo the change that triggered the addition of the test, and see whether you can reproduce the original problem with an arbitrary open file (I could trigger the problem with pagefile.sys at the time). Regards, Martin
On 4/28/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
I have a patch myself that creates an open file and uses that as the test. My reasoning is that pagefile.sys was chosen as a file that should always exist and be open, so its safe to test against, so we should just be testing against a fixture, instead. It is here, and if someone would reference a SF bug report, I'll attach to it, as well.
There must be more to the problem than just an open file. Please undo the change that triggered the addition of the test, and see whether you can reproduce the original problem with an arbitrary open file (I could trigger the problem with pagefile.sys at the time).
Regards, Martin
I'm sorry, but somehow I could not parse this. My understanding was that the unittest was meant to make sure an os.stat call would be successful on an open file, and that pagefile.sys was simply used as a known open file, which is no longer correct. If that is the case, I am unsure what problem there is with my fix of a temporary open file to test upon. -- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
I'm sorry, but somehow I could not parse this. My understanding was that the unittest was meant to make sure an os.stat call would be successful on an open file, and that pagefile.sys was simply used as a known open file, which is no longer correct.
No. The unit test was meant to test that os.stat is successful on an open file on which 2.5 reported ERROR_SHARING_VIOLATION. I believe it was not the case that it would report ERROR_SHARING_VIOLATION when stat'ing arbitrary open files, but just open files where some additional conditions must be met. I might be misremembering - I *think* I tried to write the test case just like you did, and it could not trigger the bug. I then concluded that I could not figure out what these additional conditions were, and that it *had* to be the pagefile. So please verify that your new test indeed breaks on 2.5.0 (or undo r54686). Regards, Martin
On 4/29/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
I'm sorry, but somehow I could not parse this. My understanding was that the unittest was meant to make sure an os.stat call would be successful on an open file, and that pagefile.sys was simply used as a known open file, which is no longer correct.
No. The unit test was meant to test that os.stat is successful on an open file on which 2.5 reported ERROR_SHARING_VIOLATION. I believe it was not the case that it would report ERROR_SHARING_VIOLATION when stat'ing arbitrary open files, but just open files where some additional conditions must be met. I might be misremembering - I *think* I tried to write the test case just like you did, and it could not trigger the bug. I then concluded that I could not figure out what these additional conditions were, and that it *had* to be the pagefile.
So please verify that your new test indeed breaks on 2.5.0 (or undo r54686).
Regards, Martin
Some record of this or documentation of just what conditions the tests are expecting to test against would probably be a good idea. -- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
--- "Martin v. Löwis" <martin@v.loewis.de> wrote:
There must be more to the problem than just an open file. Please undo the change that triggered the addition of the test, and see whether you can reproduce the original problem with an arbitrary open file (I could trigger the problem with pagefile.sys at the time).
Regards, Martin
--- "Calvin Spealman" <ironfroggy at gmail.com> wrote:
I'm sorry, but somehow I could not parse this. My understanding was that the unittest was meant to make sure an os.stat call would be successful on an open file, and that pagefile.sys was >simply used as a known open file, which is no longer correct. If that is the case, I am unsure what problem there is with >my fix of a temporary open file to test upon
I think the point is that the problem should be solved (stat of open file) for any arbitrary open file including pagefile.sys. After booting into Win98, I can see that I have a pagefile.sys indeed in my C drive which WinXP hides from view. While in Win98 and with the file not in use, the test passes on Win98 when looking for pagefile.sys in C drive (no complaint about file not found or access denied, even though I know that the file is not open). And the test passes for the running python.exe that is processing the test. After some googling it seems to me that this could likely be a User Rights Assignment issue of a systems file not an open file stat one, hence the Access denied error message (winerror 5) that I got in WinXP, as opposed to the File not found windows error (winerror 2) which one might expect if the pagefile.sys did not exist. And therefore if the point of the test is just to test stating an open file then the temporary file approach makes sense. If modifying a systems file with or without User Rights Assignment is a requirement then we may need a new test altogether. Regards, Khalid __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
"Khalid A. Bakr" <khabkr@yahoo.com> wrote:
--- "Martin v. Löwis" <martin@v.loewis.de> wrote:
There must be more to the problem than just an open file. Please undo the change that triggered the addition of the test, and see whether you can reproduce the original problem with an arbitrary open file (I could trigger the problem with pagefile.sys at the time).
Regards, Martin
--- "Calvin Spealman" <ironfroggy at gmail.com> wrote:
I'm sorry, but somehow I could not parse this. My understanding was that the unittest was meant to make sure an os.stat call would be successful on an open file, and that pagefile.sys was >simply used as a known open file, which is no longer correct. If that is the case, I am unsure what problem there is with >my fix of a temporary open file to test upon
I think the point is that the problem should be solved (stat of open file) for any arbitrary open file including pagefile.sys.
On Windows there is no guarantee that there will be a pagefile.sys on the C drive, or even that there exists a C drive. The test checking for the result of os.stat('C:\\pagefile.sys') is broken. Create a temporary file, open it with Python, then stat it (like you later suggest). Either that or use sys.executable . Either one would likely be fine. - Josiah
On Windows there is no guarantee that there will be a pagefile.sys on the C drive, or even that there exists a C drive. The test checking for the result of os.stat('C:\\pagefile.sys') is broken. Create a temporary file, open it with Python, then stat it (like you later suggest). Either that or use sys.executable . Either one would likely be fine.
As I said - I'm not convinced that is indeed correct. Before accepting a replacement test I would like confirmation that this test will fail on 2.5.0. You might not get ERROR_SHARING_VIOLATION in all cases of open files with 2.5.0. Regards, Martin
On 4/29/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
On Windows there is no guarantee that there will be a pagefile.sys on the C drive, or even that there exists a C drive. The test checking for the result of os.stat('C:\\pagefile.sys') is broken. Create a temporary file, open it with Python, then stat it (like you later suggest). Either that or use sys.executable . Either one would likely be fine.
As I said - I'm not convinced that is indeed correct. Before accepting a replacement test I would like confirmation that this test will fail on 2.5.0. You might not get ERROR_SHARING_VIOLATION in all cases of open files with 2.5.0.
But i am running 2.5.0 during my entire writing of this patch. I've not upgraded my laptop to 2.5.1 yet, although I verified against another installation of 2.5.1 before publishing. -- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
As I said - I'm not convinced that is indeed correct. Before accepting a replacement test I would like confirmation that this test will fail on 2.5.0. You might not get ERROR_SHARING_VIOLATION in all cases of open files with 2.5.0.
But i am running 2.5.0 during my entire writing of this patch. I've not upgraded my laptop to 2.5.1 yet, although I verified against another installation of 2.5.1 before publishing.
And you saw your test pass? Then it is not a valid test case for the bug being test, because the bug is present in 2.5.0, so your test case should fail there. Regards, Martin
On 4/29/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
As I said - I'm not convinced that is indeed correct. Before accepting a replacement test I would like confirmation that this test will fail on 2.5.0. You might not get ERROR_SHARING_VIOLATION in all cases of open files with 2.5.0.
But i am running 2.5.0 during my entire writing of this patch. I've not upgraded my laptop to 2.5.1 yet, although I verified against another installation of 2.5.1 before publishing.
And you saw your test pass? Then it is not a valid test case for the bug being test, because the bug is present in 2.5.0, so your test case should fail there.
I think I'm a little confused. Are you saying the original test should fail for me or that the test I changed it to should fail for me? The original test failed, my new one does not. As for documentating the intent of these tests, I don't think tracker items are visible enough. When I'm looking at the unittest itself, am I to always search the entire tracker for any bugs still relevent and pertaining to each test I look at? That seems contorted, and easy to miss. I'll check the tracker, and I'd like to add any information to the test itself. -- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
And you saw your test pass? Then it is not a valid test case for the bug being test, because the bug is present in 2.5.0, so your test case should fail there.
I think I'm a little confused. Are you saying the original test should fail for me or that the test I changed it to should fail for me?
If "for me" means "in 2.5.0", then yes: both the original test and the one you modified should have failed.
The original test failed, my new one does not.
Then this change is incorrect: the test should fail in 2.5.0.
As for documentating the intent of these tests, I don't think tracker items are visible enough.
Hmm. Is it asked too much to go to python.org/sf/1686475 when editing a test case named 'test_1686475'? When researching the intent of some piece of code, you actually have more information available: the set of changes that was committed together (which would include a Misc/NEWS change, and the actual change to posixmodule.c), and the log message.
When I'm looking at the unittest itself, am I to always search the entire tracker for any bugs still relevent and pertaining to each test I look at? That seems contorted, and easy to miss. I'll check the tracker, and I'd like to add any information to the test itself.
Clearly, if you think some relevant information is missing in a comment, submit a patch to add that information. I was unable to add anything, because I did not know it was missing. Regards, Martin
On 4/29/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
The original test failed, my new one does not.
Then this change is incorrect: the test should fail in 2.5.0.
I think I don't get why the test _must_ fail. If it fails, I assumed something was broken. If it failed because it was testing against a non-existant file, I assumed the test itself was broken.
As for documentating the intent of these tests, I don't think tracker items are visible enough.
Hmm. Is it asked too much to go to python.org/sf/1686475 when editing a test case named 'test_1686475'?
Maybe this is my flag for "I'm dumb sometimes", but I did wonder what the number was for and completely neglected to consider it being a ticket number!
When researching the intent of some piece of code, you actually have more information available: the set of changes that was committed together (which would include a Misc/NEWS change, and the actual change to posixmodule.c), and the log message.
When I'm looking at the unittest itself, am I to always search the entire tracker for any bugs still relevent and pertaining to each test I look at? That seems contorted, and easy to miss. I'll check the tracker, and I'd like to add any information to the test itself.
Clearly, if you think some relevant information is missing in a comment, submit a patch to add that information. I was unable to add anything, because I did not know it was missing.
I will do so. Maybe even just a link to the tracker, because of the likelihood of me not being the only person to complete miss what the number in the test name is for. ... I've read the bug report now. I see what I was missing all along. I think maybe you thought I knew of the bug report, and thus we were both confused talking on different frequencies and completely missing each other, Martin. -- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
The original test failed, my new one does not.
Then this change is incorrect: the test should fail in 2.5.0.
I think I don't get why the test _must_ fail. If it fails, I assumed something was broken.
Correct. That is the whole point of this patch: It fixes a bug in 2.5.0, and provides a test case to show that the bug was fixed. The interesting change here is *not* the test case, but the change to posixmodule.c.
If it failed because it was testing against a non-existant file, I assumed the test itself was broken.
Right. It shouldn't fail if the file is absent (it shouldn't pass in that case, either, but regrtest has no support for INCONCLUSIVE test outcomes).
I will do so. Maybe even just a link to the tracker, because of the likelihood of me not being the only person to complete miss what the number in the test name is for.
Ok. However, this pattern is quite common in the Python test suite (62 test cases, with prefixes such as test_, test_bug_, test_sf_, test_bug, test_patch_), so adding it just to this single test case may be a drop in the ocean for people unfamiliar with that convention.
I've read the bug report now. I see what I was missing all along. I think maybe you thought I knew of the bug report, and thus we were both confused talking on different frequencies and completely missing each other, Martin.
Ok! When you come up with a way to test this problem "stand-alone" (i.e. without relying on the pagefile), please submit a patch. I'll let this sit for some time, and if nothing happens, I go for Khalid's patch before 2.5.2 is released (which is still months ahead). Regards, Martin
On 4/29/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
Right. It shouldn't fail if the file is absent (it shouldn't pass in that case, either, but regrtest has no support for INCONCLUSIVE test outcomes).
Perhaps that could become part of the improvements made through test.test_support.TestCase?
Ok. However, this pattern is quite common in the Python test suite (62 test cases, with prefixes such as test_, test_bug_, test_sf_, test_bug, test_patch_), so adding it just to this single test case may be a drop in the ocean for people unfamiliar with that convention.
Very true, but maybe more tests could have the more descriptive names, then. For example, I would have known what it meant if the test name prefix was test_sf_ instead of just test_. Changing the names shouldn't interfere with anything else, so if I rename them in an effort to help the next guy, would that be accepted?
Ok! When you come up with a way to test this problem "stand-alone" (i.e. without relying on the pagefile), please submit a patch. I'll let this sit for some time, and if nothing happens, I go for Khalid's patch before 2.5.2 is released (which is still months ahead).
Now that I have the full picture, I have less motivation about it. Although, I am curious what is different about the situation where pagefile.sys could not be stat'ed in 2.5.0 but other open files could. -- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
Calvin Spealman schrieb:
On 4/29/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
Right. It shouldn't fail if the file is absent (it shouldn't pass in that case, either, but regrtest has no support for INCONCLUSIVE test outcomes).
Perhaps that could become part of the improvements made through test.test_support.TestCase?
Sure. I think this is PEP material - I would like to declare "expected failure" as well.
Very true, but maybe more tests could have the more descriptive names, then. For example, I would have known what it meant if the test name prefix was test_sf_ instead of just test_. Changing the names shouldn't interfere with anything else, so if I rename them in an effort to help the next guy, would that be accepted?
That would be fine (of course, we move away from SF, so these method names, at some point, will trigger synapses only for old-timers that still remember sourceforge; the bug IDs will remain constant in the next tracker).
Now that I have the full picture, I have less motivation about it. Although, I am curious what is different about the situation where pagefile.sys could not be stat'ed in 2.5.0 but other open files could.
The error Windows reports is ERROR_SHARING_VIOLATION. I never understood sharing fully, but it may be that if the file is opened in "exclusive sharing", stat'ing it may fail. I personally consider it a bug in Windows that you cannot get file attributes if some other process has opened it. Exclusive access should only restrict access to file contents, but not file attributes. Regards, Martin
On 4/29/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
Calvin Spealman schrieb:
On 4/29/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
Right. It shouldn't fail if the file is absent (it shouldn't pass in that case, either, but regrtest has no support for INCONCLUSIVE test outcomes).
Perhaps that could become part of the improvements made through test.test_support.TestCase?
Sure. I think this is PEP material - I would like to declare "expected failure" as well.
I would second that. Twisted's trial system does a lot of this, already. I suppose that this conversation would end inside this thread now; it is officially off topic of the thread. But, yes, I would love to see it. test_support could definately act as a testbed for new things before moving to unittest or the unittest replacement.
Very true, but maybe more tests could have the more descriptive names, then. For example, I would have known what it meant if the test name prefix was test_sf_ instead of just test_. Changing the names shouldn't interfere with anything else, so if I rename them in an effort to help the next guy, would that be accepted?
That would be fine (of course, we move away from SF, so these method names, at some point, will trigger synapses only for old-timers that still remember sourceforge; the bug IDs will remain constant in the next tracker).
So test_bug_ then.
Now that I have the full picture, I have less motivation about it. Although, I am curious what is different about the situation where pagefile.sys could not be stat'ed in 2.5.0 but other open files could.
The error Windows reports is ERROR_SHARING_VIOLATION. I never understood sharing fully, but it may be that if the file is opened in "exclusive sharing", stat'ing it may fail.
I personally consider it a bug in Windows that you cannot get file attributes if some other process has opened it. Exclusive access should only restrict access to file contents, but not file attributes.
Perhaps there are things about the implementation of file operations in "exclusive sharing" mode of files that can not guarantee consistant, reliable, or correct results of a stat call during that time, and the decision was simply "If it can't be correct, it can't be at all." -- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
Hi Martin, On 4/29/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
The error Windows reports is ERROR_SHARING_VIOLATION. I never understood sharing fully, but it may be that if the file is opened in "exclusive sharing", stat'ing it may fail.
Sharing is actually very easy. If you didn't specify SHARE_READ/SHARE_WRITE/SHARE_DELETE when opening the file and you succeeded, then any process can't read/write/delete the file until you close the handle.
I personally consider it a bug in Windows that you cannot get file attributes if some other process has opened it. Exclusive access should only restrict access to file contents, but not file attributes.
After doing some research I found that it seems to be impossible to use CreateFile for a file that doesn't have SHARE_READ. I played with different combinations and with FLAG_BACKUP_SEMANTICS and nothing helped. However on Windows there's still a possibility to read attributes: use FindFirstFile. _WIN32_FIND_DATA structure seems to have all the necessary fields (attributes, file times, size and full/short filename), and FindFirstFile doesn't care about sharing at all...
After doing some research I found that it seems to be impossible to use CreateFile for a file that doesn't have SHARE_READ. I played with different combinations and with FLAG_BACKUP_SEMANTICS and nothing helped. However on Windows there's still a possibility to read attributes: use FindFirstFile. _WIN32_FIND_DATA structure seems to have all the necessary fields (attributes, file times, size and full/short filename), and FindFirstFile doesn't care about sharing at all...
So what about GetFileAttributesEx? What are the conditions under which I can successfully invoke it? Regards, Martin
On 5/1/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
After doing some research I found that it seems to be impossible to use CreateFile for a file that doesn't have SHARE_READ. I played with different combinations and with FLAG_BACKUP_SEMANTICS and nothing helped. However on Windows there's still a possibility to read attributes: use FindFirstFile. _WIN32_FIND_DATA structure seems to have all the necessary fields (attributes, file times, size and full/short filename), and FindFirstFile doesn't care about sharing at all... So what about GetFileAttributesEx? What are the conditions under which I can successfully invoke it?
Seems to have the same problems as with CreateFile(...): // 1.cc #include <windows.h> #include <stdio.h> int main(int argc, char** argv) { WIN32_FILE_ATTRIBUTE_DATA data; if(!GetFileAttributesEx("pagefile.sys", GetFileExInfoStandard, (LPVOID)&data)) { char buffer[1024]; FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(), 0, buffer, 1024, NULL); printf("Error %d: %s\n", GetLastError(), buffer); return 1; } printf("Success\n"); return 0; } // 2.cc #include <windows.h> #include <stdio.h> int main(int argc, char** argv) { WIN32_FIND_DATA data; if(INVALID_HANDLE_VALUE == FindFirstFile("pagefile.sys", &data)) { char buffer[1024]; FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError(), 0, buffer, 1024, NULL); printf("Error %d: %s\n", GetLastError(), buffer); return 1; } printf("Success\n"); return 0; } C:\>C:\3\1.exe Error 32: The process cannot access the file because it is being used by another process. C:\>C:\3\2.exe Success
Alexey Borzenkov schrieb:
On 5/1/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
After doing some research I found that it seems to be impossible to use CreateFile for a file that doesn't have SHARE_READ. So what about GetFileAttributesEx? What are the conditions under which I can successfully invoke it?
Seems to have the same problems as with CreateFile(...):
That code only tests it for pagefile.sys. My question was about open handles in general. Both Calvin Spealman and I found that you cannot reproduce the problem when you, in Python 2.5.0, open a file, and then try to os.stat() it - even though, in Python 2.5.0, os.stat() will perform GetFileAttributesEx. So even though we opened the file with not passing any sharing flags, we could still do GetFileAttributesEx on it. I now studied the CRT sources, and it seems that if you use a regular open() call, the CRT will pass FILE_SHARE_READ | FILE_SHARE_WRITE to CreateFile. You would have to use _sopen in the CRT to create any kind of sharing conflict, and that isn't exposed in Python. So I guess we need continue using pagefile.sys as a test case. Regards, Martin
On 5/1/07, "Martin v. Löwis" <martin@v.loewis.de> wrote:
That code only tests it for pagefile.sys. My question was about open handles in general. Both Calvin Spealman and I found that you cannot reproduce the problem when you, in Python 2.5.0, open a file, and then try to os.stat() it - even though, in Python 2.5.0, os.stat() will perform GetFileAttributesEx. So even though we opened the file with not passing any sharing flags, we could still do GetFileAttributesEx on it.
I now studied the CRT sources, and it seems that if you use a regular open() call, the CRT will pass FILE_SHARE_READ | FILE_SHARE_WRITE to CreateFile. You would have to use _sopen in the CRT to create any kind of sharing conflict, and that isn't exposed in Python.
Wow, I'm very sorry, I didn't realize how much special pagefile.sys and hiberfil.sys are. As it turns out, even if you create a file with no sharing allowed, you can still open it with backup semantics in other processes, and thus can use GetFileAttributesEx, GetFileTime, etc. The file pagefile.sys seems almost magical then, I don't understand how it's opened to behave like that. The difference is also immediately visible if you try to open Properties of pagefile.sys, you won't even see Security tab there (even when I create file something.txt and then remove all ACLs, including SYSTEM, I can't access the file, but I can see Security tab and can grant myself permissions back), it looks like all kinds of opening that file are denied. Maybe this is a special security feature, so that no process could access swapped pages (otherwise it could be possible with backup semantics). Thus you can't access the file itself, you can only access containing directory.
So I guess we need continue using pagefile.sys as a test case.
Seems to be true, it's just maybe it shouldn't be hardcoded to C:\ There's REG_MULTI_SZ PagingFiles in "HKLM\System\CurrentControlSet\Control\Session Manager\Memory Management", btw. The format seems to be "filename minmbsize maxmbsize" for every line. Best regards, Alexey.
Hm, I fail to see the importance of a special regression test for that peculiar file then with its special magical OS properties. Why not focus our attention on real, user generated files?. -----Original Message----- Wow, I'm very sorry, I didn't realize how much special pagefile.sys and hiberfil.sys are. As it turns out, even if you create a file with no sharing allowed, you can still open it with backup semantics in other processes, and thus can use GetFileAttributesEx, GetFileTime, etc. The file pagefile.sys seems almost magical then, I don't understand how it's opened to behave like that. The difference is also immediately visible if you try to open Properties of pagefile.sys, you won't even see Security tab there (even when I create file something.txt and then remove all ACLs, including SYSTEM, I can't access the file, but I can see Security tab and can grant myself permissions back), it looks like all kinds of opening that file are denied. Maybe this is a special security feature, so that no process could access swapped pages (otherwise it could be possible with backup semantics). Thus you can't access the file itself, you can only access containing directory.
So I guess we need continue using pagefile.sys as a test case.
Seems to be true, it's just maybe it shouldn't be hardcoded to C:\ There's REG_MULTI_SZ PagingFiles in "HKLM\System\CurrentControlSet\Control\Session Manager\Memory Management", btw. The format seems to be "filename minmbsize maxmbsize" for every line.
Kristján Valur Jónsson schrieb:
Hm, I fail to see the importance of a special regression test for that peculiar file then with its special magical OS properties. Why not focus our attention on real, user generated files?.
Because real users really had real problems with this very real file, or else they had not reported that as a real bug, really. Are you proposing to unfix the bug? Regards, Martin
On 5/1/07, Kristján Valur Jónsson <kristjan@ccpgames.com> wrote:
Hm, I fail to see the importance of a special regression test for that peculiar file then with its special magical OS properties. Why not focus our attention on real, user generated files?.
(Try to stick to the posting conventions and reply under the actual segments you respond to.) Not all the user generated files are directly from python. Consider all the extension libraries that can do anything they want opening files on lower levels. For example, database files are likely to have different sharing flags than the default. I'm not sure if sqlite, for example, may or may not have such problems. Martin: Would tests that use ctypes do do the open directly be acceptable ways of solving this?
-----Original Message-----
Wow, I'm very sorry, I didn't realize how much special pagefile.sys and hiberfil.sys are. As it turns out, even if you create a file with no sharing allowed, you can still open it with backup semantics in other processes, and thus can use GetFileAttributesEx, GetFileTime, etc. The file pagefile.sys seems almost magical then, I don't understand how it's opened to behave like that. The difference is also immediately visible if you try to open Properties of pagefile.sys, you won't even see Security tab there (even when I create file something.txt and then remove all ACLs, including SYSTEM, I can't access the file, but I can see Security tab and can grant myself permissions back), it looks like all kinds of opening that file are denied. Maybe this is a special security feature, so that no process could access swapped pages (otherwise it could be possible with backup semantics). Thus you can't access the file itself, you can only access containing directory.
So I guess we need continue using pagefile.sys as a test case.
Seems to be true, it's just maybe it shouldn't be hardcoded to C:\ There's REG_MULTI_SZ PagingFiles in "HKLM\System\CurrentControlSet\Control\Session Manager\Memory Management", btw. The format seems to be "filename minmbsize maxmbsize" for every line.
_______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/ironfroggy%40gmail.com
-- Read my blog! I depend on your acceptance of my opinion! I am interesting! http://ironfroggy-code.blogspot.com/
After some googling it seems to me that this could likely be a User Rights Assignment issue of a systems file not an open file stat one, hence the Access denied error message (winerror 5) that I got in WinXP, as opposed to the File not found windows error (winerror 2) which one might expect if the pagefile.sys did not exist.
Ah - I didn't check what the error number is. If you can get ERROR_ACCESS_DENIED, then most likely we need to check for ERROR_FILE_NOT_FOUND as well (as your original patch did) - it would be good if somebody confirmed that the modified test indeed passes/gets skipped when the pagefile is not on C:
And therefore if the point of the test is just to test stating an open file then the temporary file approach makes sense. If modifying a systems file with or without User Rights Assignment is a requirement then we may need a new test altogether.
The test should ideally verify that you can stat all open files in 2.5 that you could also stat in 2.4. If you get ERROR_ACCESS_DENIED when stat'ing c:\pagefile.sys in 2.5, I would *hope* that you get a similar error from 2.4. If 2.4 could stat c:\pagefile.sys and 2.5 gives ERROR_ACCESS_DENIED, then the bug still isn't fixed. Regards, Martin
participants (8)
-
"Martin v. Löwis" -
Alexey Borzenkov -
Calvin Spealman -
Jean-Paul Calderone -
Josiah Carlson -
Khalid A. Bakr -
Kristján Valur Jónsson -
Raghuram Devarakonda