Expose linux extended filesystem attributes

[neologix_free.fr]

Here's a review.

http://bugs.python.org/review/12720/diff/3188/9927
File Doc/library/os.rst (right):

http://bugs.python.org/review/12720/diff/3188/9927#newcode1919
Doc/library/os.rst:1919: must be a bytes or str with no embedded NULLs. If it is
str, it is encoding
I didn't quite follow the recent discussion, but should this be NUL or NULL ;-)

http://bugs.python.org/review/12720/diff/3188/9927#newcode1919
Doc/library/os.rst:1919: must be a bytes or str with no embedded NULLs. If it is
str, it is encoding
typo: encoded

http://bugs.python.org/review/12720/diff/3188/9928
File Lib/test/test_os.py (right):

http://bugs.python.org/review/12720/diff/3188/9928#newcode1526
Lib/test/test_os.py:1526: def setxattr_flags_broken():
support.requires_linux_version(2.6.39) decorator would come in handy here.

http://bugs.python.org/review/12720/diff/3188/9928#newcode1552
Lib/test/test_os.py:1552: if setxattr_flags_bad:
It would be simpler to just skip these tests on Linux < 2.6.39: those kernels
will disappear slowly, and some distributions (e.g. RHEL) backport patches, so
those tests might fail on such kernels.

http://bugs.python.org/review/12720/diff/3188/9929
File Modules/posixmodule.c (right):

http://bugs.python.org/review/12720/diff/3188/9929#newcode111
Modules/posixmodule.c:111: #include <attr/xattr.h>
You might want to release the GIL around those syscalls.

http://bugs.python.org/review/12720/diff/3188/9929#newcode9966
Modules/posixmodule.c:9966: 
Here, each `get` syscall is called twice, to pre-compute the buffer size: while
I understand we don't want to allocate an XATTR_SIZE_MAX (64K) buffer each time,
we could maybe start with a reasonably-sized buffers size (let's say 128), and
if the call returns ERANGE, then try with XATTR_SIZE_MAX: this would avoid the
overhead of calling the syscall twice in 99% of cases.

http://bugs.python.org/review/12720/diff/3188/9929#newcode10179
Modules/posixmodule.c:10179: len = list(path, NULL, 0);
Same thing here.
Try with a reasonable size, and on ERANGE, try with XATTR_LIST_MAX.

[AntoinePitrou]

http://bugs.python.org/review/12720/diff/3263/10150
File Modules/posixmodule.c (right):

http://bugs.python.org/review/12720/diff/3263/10150#newcode9988
Modules/posixmodule.c:9988: _PyBytes_Resize(&value, len);
You should return 0 if _PyBytes_Resize() fails.

http://bugs.python.org/review/12720/diff/3263/10150#newcode10066
Modules/posixmodule.c:10066: if (!PyArg_ParseTuple(args, "iO&:fgetxattr", &fd,
PyUnicode_FSConverter, &name))
Typecode "i" expects an int * but you are giving it a long *.
Also, fgetxattr takes an int, not a long.

http://bugs.python.org/review/12720/diff/3263/10150#newcode10068
Modules/posixmodule.c:10068: res = getxattr_common((const char *)fd, name,
wrap_fgetxattr);
I'm no C lawyer, but maybe you should be casting to unsigned before casting to a
pointer. Otherwise I don't know if there might be some sign extension.
Of course, you may change the helper functions to take a Py_uintptr_t instead.

http://bugs.python.org/review/12720/diff/3263/10150#newcode10223
Modules/posixmodule.c:10223: *buf = PyMem_MALLOC(buf_size);
You should check if *buf is NULL.

http://bugs.python.org/review/12720/diff/3263/10150#newcode10326
Modules/posixmodule.c:10326: if (!PyArg_ParseTuple(args, "i:flistxattr", &fd))
Same as above.

[Benjamin Peterson]

http://bugs.python.org/review/12720/diff/3263/10150
File Modules/posixmodule.c (right):

http://bugs.python.org/review/12720/diff/3263/10150#newcode9988
Modules/posixmodule.c:9988: _PyBytes_Resize(&value, len);
On 2011/08/28 18:34:13, AntoinePitrou wrote:
> You should return 0 if _PyBytes_Resize() fails.

Shrinking cannot possibly fail, though.

[neologix_free.fr]

The FD -> char * cast is really ugly, and it's implementation-defined behavior.
How about using a tagged union instead?

http://bugs.python.org/review/12720/diff/3263/10150
File Modules/posixmodule.c (right):

http://bugs.python.org/review/12720/diff/3263/10150#newcode10002
Modules/posixmodule.c:10002: if (!try_getxattr(path, name, get, 128, &value))
Don't you mean XATTR_BUF_SIZE instead of hard-coded 128?

http://bugs.python.org/review/12720/diff/3263/10150#newcode10068
Modules/posixmodule.c:10068: res = getxattr_common((const char *)fd, name,
wrap_fgetxattr);
On 2011/08/28 18:34:13, AntoinePitrou wrote:
> I'm no C lawyer, but maybe you should be casting to unsigned before casting to
a
> pointer. Otherwise I don't know if there might be some sign extension.

Correct. But the problem is that this is really implementation behavior: AFAICT,
this could perfectly break on a specific platform/compiler. Plus, it's really
ugly :-(

http://bugs.python.org/review/12720/diff/3263/10150#newcode10243
Modules/posixmodule.c:10243: len = try_listxattr(path, list, 256, &buf);
XATTR_BUF_SIZE

[Benjamin Peterson]

http://bugs.python.org/review/12720/diff/3263/10150
File Modules/posixmodule.c (right):

http://bugs.python.org/review/12720/diff/3263/10150#newcode10002
Modules/posixmodule.c:10002: if (!try_getxattr(path, name, get, 128, &value))
On 2011/08/28 20:02:17, Charles-François Natali wrote:
> Don't you mean XATTR_BUF_SIZE instead of hard-coded 128?

Actually I meant to remove the XATTR_BUF_SIZE define.

http://bugs.python.org/review/12720/diff/3263/10150#newcode10068
Modules/posixmodule.c:10068: res = getxattr_common((const char *)fd, name,
wrap_fgetxattr);
On 2011/08/28 20:02:17, Charles-François Natali wrote:
> On 2011/08/28 18:34:13, AntoinePitrou wrote:
> > I'm no C lawyer, but maybe you should be casting to unsigned before casting
to
> a
> > pointer. Otherwise I don't know if there might be some sign extension.
> 
> Correct. But the problem is that this is really implementation behavior:
AFAICT,
> this could perfectly break on a specific platform/compiler. Plus, it's really
> ugly :-(

In my latest patch I have (const char *)(Py_uintptr_t)fd. Where is that
implementation defined?