Re-implementation of the stat module in C #55225
Comments
|
Solaris has an additional kind of special files named doors. The standard headers define the following macro: #define S_ISDOOR(mode) (((mode)&0xF000) == 0xd000)Perhaps it would be nice to include the equivalent in the stat module. (although funnily even the "stat" command doesn't recognize them and displayed "weird file" instead: $ stat /var/run/syslog_door
File: `/var/run/syslog_door'
Size: 0 Blocks: 0 IO Block: 0 weird file
Device: 8bc0000h/146538496d Inode: 44 Links: 1
Access: (0644/Drw-r--r--) Uid: ( 0/ root) Gid: ( 0/ root)
Access: 2011-01-26 11:52:34.332492612 +0100
Modify: 2011-01-26 11:52:34.332492612 +0100
Change: 2011-01-26 11:52:34.332499428 +0100
) |
pitrou
added
stdlib
|
Thanks for the feature request. It seems trivial to implement, but not trivial to test :). I assign this to myself. I will work on it after Mercurial migration is finished. Getting impatient :). |
You can test against an existing file, such as "/var/run/syslog_door" |
|
I do know, but when you are working inside a zone, I am not sure you can count of that file being always present. Syslog even could be disabled or not available inside a zone. I am thinking about how to manage OS's with no support for doors. Instead of conditionally compile the new STAT, I would rather have the function always available, but returning FALSE when the OS doesn't support doors. But what happen if you mount a Solaris filesystem in a nonsolaris machine?. Let say, a Solaris ZFS filesystem with doors, under linux/*bsd + ZFS?. So maybe would be better to not compile that function in those OS's, instead of "lying" in corner cases. Not decided yet. I accept ideas. |
Then you can just skip the test.
Lib/stat.py is a pure Python module. There is no compilation to do.
Right.
I guess S_ISDOOR would return True. |
|
Given the behaviour of Solaris DOORS, the flags selected are very cleverly chosen. Sensible engineering :). But now I am wondering... Which organization defines flags like S_IFSOCK or S_IFIFO?. Posix members?. I am worried about flag collision between OSs. For instance, if DOORS were a real flag defined in Solaris, how other OSs would know to not reuse that flag for some other purpose? :). Your comment is very valuable and solve my doubts about it. Implementation is truly trivial and could be supported everywhere. |
They are defined unconditionally in Lib/stat.py.
Which flag? S_ISDOOR uses an (unlikely) combination of existing flags. |
|
Antoine, I am not talking about python, I am talking about the UNIX standarization process. In particular, how a new flag (for filesystems) in an OS can be "skipped" and not being reused for some other purpuse in other OS. Off topic for python, but curious to know anyway :). |
|
Jesus, yes, it's totally possible that POSIX might define: (stat.S_IFSOCK + stat.S_IFIFO) to mean something other than S_IDOOR. However, the POSIX committee |
|
I am looking at Linux :-). Anyway the feedback has been very useful. I will implement this as soon as mercurial switch is done. |
I'd rather see this exposed from the posix module than the stat module For testing, if S_ISDOR is available but /var/run/name_service_door is |
|
Am 26.01.2011 14:14, schrieb Jesús Cea Avión:
There is no standards body defining the S_IFMT numerical values. http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html The Python stat module is flawed in assuming it knows the numeric values |
|
Martin, what if C posix module (or whoever) would export the symbolic constants, and update "stat.py" to use those symbolic constants?. Do you think that would be an improvement?. |
Improvement compared to what? The status quo? Certainly. |
|
Martin, I guess "stat" deprecation could require a few years and it would be an extra incompatibility burden between 2.7 and 3.x. Beside the symbolic constants, why would you see "stat" deprecated?. |
|
Am 31.01.2011 12:46, schrieb Jesús Cea Avión:
Because it doesn't provide any useful functionality, and confuses The other constants and functions (S_*) are plainly non-portable. |
Are there actual bugs with that or is it merely a theoretical concern? |
Neither, nor (i.e. it's in the middle). I'm not aware of an actual Looking in more detail: for the S_IFMT flags, OSX/Darwin/FreeBSD defines |
|
After reviewing the code, I agree with Martin v. Löwis that "stat.py" is a joke. Neither can we trust the numerical constants (each OS is free to choose a different meaning) neither the code in "filemode()" can cope with modes with more than a bit active (like DOORS). I think all this code should be moved to a C module, able to access the underlining real constant definitions. What do you think?. Time to deprecate this and move the functionality into posix module, as suggested by Martin?. |
|
The stat module (or whatever it is going to be in Python 3.4) is missing more checks than S_ISDOOR: # Solaris
S_ISDOOR()
S_ISPORT()
# POSIX 1.b real-time extension
S_ISMSG()
S_ISSEM()
S_ISSHM()# whiteout, translucent file systems |
|
I have done some research. The POSIX 1.b extensions aren't used on any supported system (neither Linux nor BSD, Solaris or AIX). random832@fastmail.us has compiled a list of even more file types: Heirloom toolchest "ls" supports: Of these, GNU coreutils ls only supports doors and whiteouts. Chasing after a random hunch (something about AIX), I found these: http://cd.textfiles.com/transameritech2/EXTRAS/JOVE-4.6/ASK.C http://lists.gnu.org/archive/html/bug-gnulib/2012-12/msg00084.html https://github.com/gagern/gnulib/blob/master/tests/test-sys_stat.c http://lists.gnu.org/archive/html/bug-gnulib/2004-08/msg00017.html http://doiso.googlecode.com/svn/trunk/Source/mkisofs-1.12b5/include/statdefs.h http://www.opensource.apple.com/source/gnutar/gnutar-450/gnutar/lib/sys_stat_.h |
|
I have created a C implementation of the stat module for Python 3.4. It implements all current features, handling for DOOR, PORT, WHT and a fix for bpo-17913. The first half of the file has lots of #ifndef checks. I'm not sure if they are really required. I guess they are required for the tarfile module on Windows. |
|
Christian, could you post it as a mercurial diff for review? Shouldn't we also remove the Python version? |
|
We've been *adding* python implementations for other modules, so I don't see why we would remove this one. |
Well, the one reason is that the C constants aren't accessible from |
Because it's buggy, and cannot be implemented correctly in python. |
|
"Cannot be implemented correctly in Python" is a darned good reason :) |
|
If we use the C language, I would prefer to only expose what does really exist, and not add "fake" functions like: +#ifndef S_ISDOOR So in: +static PyMethodDef stat_methods[] = { I expect something similar to what is done in posixmodule.c: static PyMethodDef stat_methods[] = {
{"S_ISDIR", stat_S_ISDIR, METH_O, stat_S_ISDIR_doc},
...
#ifdef S_ISDOOR
{"S_ISDOOR", stat_S_ISDOOR, METH_O, stat_S_ISDOOR_doc},
#endif
{"filemode", stat_filemode, METH_O, stat_filemode_doc},
{NULL, NULL} /* sentinel */
}; |
Oh, your C module is called "stat", as the "stat" module implemented in Python (Lib/stat.py). What is your plan? Replace completly the Python module with the C module? It may be nice to keep the Python module for compatibility with other Python implementations, and test both implementations. |
|
AP has started to review my patch. I'm not yet sure how we should handle constants and functions when the platform doesn't provide them. For example Windows doesn't have any S_IS*() function like macros and doesn't provide a S_IFBLK constant. We have three possibilities here:
I'm against 1) because it breaks backwards compatibility and makes the module harder to use. I like to follow 3) for all S_I*, SF_* and UF_* macros and functions that are currently provided by the stat module and 2) for new constants and functions such as S_ISDOOR(). |
|
New patch with extensive tests for the stat module. The tests are testing both the new C module and the old stat.py module. |
|
The latest patch comes with more comments and documentation updates. The C implementation defines all existing constants to the hard coded values from stat.py if the platform doesn't provide them. The approach keeps maximum backward compatibility with the old stat module. I have also added the new constants and functions to the pure Python implementation for maximum compatibility with other Python implementation. |
|
I still fail to understand why you just don't ditch the Python implementation. |
|
New changeset f8ff61f44aca by Christian Heimes in branch '3.3': New changeset d15aee50e4a0 by Christian Heimes in branch 'default': |
|
New patch People demand a _stat module in C and now they are getting a _stat module in C. |
|
New changeset 420f70a22b9d by Christian Heimes in branch 'default': |
|
buf[9] is not initialized in stat_filemode(). Use a shorter buffer (9
|
|
All 10 chars are set: buf[0] = filetype(mode); buf[0] is set by filetype(). |
|
New changeset e5427b0b2bf7 by Victor Stinner in branch 'default': |
Ah ok, fine, I missed the "&buf[1]" hack. |
@christian: Can you please review this commit? By the way, mode_t is also defined in import.c: #ifdef MS_WINDOWS
/* for stat.st_mode */
typedef unsigned short mode_t;
/* for _mkdir */
#include <direct.h>
#endifAnd stat_filemode() should detect integer overflow. mode_t is a 32-bit unsigned integer on Linux, and now a 16-bit integer on Windows, whereas stat_filemode() uses an unsigned long (which 32 bit on Windows, and 32 or 64 bits on Linux). |
Attached stat_mode_overflow.patch should fix this issue. (I would also suggest to inline fileperm() into stat_filemode(), or pass buf instead of &buf[1]. But you may not agree, as you want :-)) |
|
My changeset e5427b0b2bf7 is not enough: "import _stat" still fail on Windows. See for example: ====================================================================== Traceback (most recent call last):
File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_stat.py", line 128, in test_directory
st_mode, modestr = self.get_mode()
File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_stat.py", line 67, in get_mode
modestr = self.statmod.filemode(st_mode)
AttributeError: 'NoneType' object has no attribute 'filemode'====================================================================== Traceback (most recent call last):
File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_stat.py", line 119, in test_mode
st_mode, modestr = self.get_mode()
File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_stat.py", line 67, in get_mode
modestr = self.statmod.filemode(st_mode)
AttributeError: 'NoneType' object has no attribute 'filemode'====================================================================== Traceback (most recent call last):
File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_stat.py", line 169, in test_module_attributes
modvalue = getattr(self.statmod, key)
AttributeError: 'NoneType' object has no attribute 'ST_DEV' |
|
test_stat.test_devices() fail on Solaris: it looks like os.devnull is a symlink. You should probably use os.stat() instead of os.lstat() for this specific test. ====================================================================== Traceback (most recent call last):
File "/home/cpython/buildslave/cc-32/3.x.snakebite-sol10-sparc-cc-32/build/Lib/test/test_stat.py", line 157, in test_devices
self.assertEqual(modestr[0], 'c')
AssertionError: 'l' != 'c'
- l
+ c |
|
I have addressed the Windows build issue in http://hg.python.org/cpython/rev/838f04e5a690 and the failing test on Solaris in http://hg.python.org/cpython/rev/6c23ca1982b3 (also 2.7 and default). |
|
New changeset 44c8a9d80595 by Victor Stinner in branch 'default': |
|
New changeset 75bc0ae02bcd by Christian Heimes in branch 'default': |
|
Can we re-close this issue? Or is there still something to do? |
|
Let's close it. |
|
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: