Issue 14626: os module: use keyword-only arguments for dir_fd and nofollow to reduce function count

storchaka

Wonderful work, Larry! This is not all of the review, the code requires careful consideration. ...

11 months, 3 weeks ago #1

larry

Patch coming right up. http://bugs.python.org/review/14626/diff/5009/Lib/os.py File Lib/os.py (right): http://bugs.python.org/review/14626/diff/5009/Lib/os.py#newcode365 Lib/os.py:365: if st.S_ISDIR(stat(dir_fd=topfd, path=name).st_mode): On 2012/05/30 ...

11 months, 2 weeks ago #2

Patch coming right up.

http://bugs.python.org/review/14626/diff/5009/Lib/os.py
File Lib/os.py (right):

http://bugs.python.org/review/14626/diff/5009/Lib/os.py#newcode365
Lib/os.py:365: if st.S_ISDIR(stat(dir_fd=topfd, path=name).st_mode):
On 2012/05/30 21:29:27, storchaka wrote:
> IMHO `stat(name, dir_fd=topfd)` looks better.
> See `open(name, O_RDONLY, dir_fd=topfd)` below.

This was kind of an experiment.  Note that all the POSIX functions that take a
dir_fd take it just before the path; I was trying it this way to see if it
seemed any clearer.
Ultimately I agree that it isn't any better.
tl;dr: Done.

http://bugs.python.org/review/14626/diff/5009/Lib/os.py#newcode383
Lib/os.py:383: orig_st = stat(dir_fd=topfd, path=name,
follow_symlinks=followlinks)
On 2012/05/30 21:29:27, storchaka wrote:
> `follow_symlinks=followlinks` -- isn't it strange looks?

How so?

http://bugs.python.org/review/14626/diff/5009/Lib/shutil.py
File Lib/shutil.py (right):

http://bugs.python.org/review/14626/diff/5009/Lib/shutil.py#newcode144
Lib/shutil.py:144: utime_func = os.utime if hasattr(os, 'utime') else _nop
On 2012/05/30 21:29:27, storchaka wrote:
> Isn't all of these functions replace the calls with argument
"follow_symlinks"?
> 
>     symlinks = symlinks and os.path.islink(src) and os.path.islink(dst)
>     st = os.stat(src, follow_symlinks=not symlinks)
>     mode = stat.S_IMODE(st.st_mode)
>     if hasattr(os, 'utime'):
>         os.utime(dst, ns=(st.st_atime_ns, st.st_mtime_ns), follow_symlinks=not
> symlinks)
>     if hasattr(os, 'chmod'):
>         os.chmod(dst, mode, follow_symlinks=not symlinks)
>     ...

Good idea!  We still need to handle chflags being unavailable on Windows, but
other than that the code is much nicer now.

http://bugs.python.org/review/14626/diff/5009/Lib/shutil.py#newcode180
Lib/shutil.py:180: for attr in os.listxattr(src, follow_symlinks=symlinks):
On 2012/05/30 21:29:27, storchaka wrote:
> follow_symlinks=not symlinks
Done.

http://bugs.python.org/review/14626/diff/5009/Lib/shutil.py#newcode182
Lib/shutil.py:182: os.setxattr(dst, attr, os.getxattr(src, attr,
follow_symlinks=symlinks), follow_symlinks=symlinks)
On 2012/05/30 21:29:27, storchaka wrote:
> follow_symlinks=not symlinks
> 
> Line is too long. Will be better to use named variable for the result of
> os.getxattr().

There are a lot of > 80 col lines.  You don't need to tell me.  I'm leaving them
for now as I don't want to bother making the code pretty until it stops
changing.

I'll fix them before I make a patch I seriously want to check in.  (This patch
clearly isn't ready yet; the docstrings are a bit messy, and I haven't fixed the
docs at all.)

http://bugs.python.org/review/14626/diff/5009/Modules/posixmodule.c
File Modules/posixmodule.c (right):

http://bugs.python.org/review/14626/diff/5009/Modules/posixmodule.c#newcode635
Modules/posixmodule.c:635: PyErr_Format(PyExc_ValueError, "%s: must specify
either valid path or valid fd", function_name);
On 2012/05/30 21:29:27, storchaka wrote:
> Line is too long.

See previous comment.

http://bugs.python.org/review/14626/diff/5009/Modules/posixmodule.c#newcode2222
Modules/posixmodule.c:2222: "access(path, mode, *, dir_fd=AT_FDCWD,
effective_ids=0, follow_symlinks=True)\n\n\
On 2012/05/30 21:29:27, storchaka wrote:
> May be use None for dir_fd? This will require a special converter. Error check
> can be done in this converter instead of dir_fd_specified()/fd_specified().
> 
> effective_ids=False

Okay.  I like the idea of accepting "None" for dir_fd by default.  I was mixed
on doing that for "fd" too, as passing in -1 to represent an invalid fd is a
well-established convention.  But I've tried it and I think it's an improvement.
 It's certainly a lot prettier in the docstring!

However: the error check can't be done *in* the converter, because the converter
doesn't know whether or not the argument is locally supported.  So I just switch
converters.  The new idiom here is to use two converters; one accepts any legal
arguments, and the other only accepts None and the default argument.  Then I
pass in the correct converter based on the appropriate HAVE_ macro.

http://bugs.python.org/review/14626/diff/5009/Modules/posixmodule.c#newcode2296
Modules/posixmodule.c:2296: if ((dir_fd != DEFAULT_DIR_FD) ||
On 2012/05/30 21:29:27, storchaka wrote:
> If faccessat exists it can be used uncoditionally.

My general approach was to use the weakest function that achieved the
functionality.  If all you need is access() then I'll use that.  It means all
the code for the different functions reads the same, as well as perhaps being
marginally faster.

However!  I've just discovered that fchownat doesn't actually support
AT_SYMLINK_NOFOLLOW.  They list it in the documentation as some sort of
practical joke; if you read the fine print it says it's unsupported.  (Really! 
It's like "Here's a parameter which DOESN'T WORK.  Enjoy!")  So I'm
restructuring the code to explicitly use the weakest/oldest function that will
suffice, here and elsewhere.

http://bugs.python.org/review/14626/diff/5009/Modules/posixmodule.c#newcode3011
Modules/posixmodule.c:3011: follow_symlinks ? 0 : AT_SYMLINK_NOFOLLOW);
On 2012/05/30 21:29:27, storchaka wrote:
> follow_symlinks ? AT_SYMLINK_FOLLOW : 0

Thanks!  I'll make a pass and make sure all of these match.  I gotta say, having
flags for both FOLLOW and NOFOLLOW is an API hate crime.

http://bugs.python.org/review/14626/diff/5009/Modules/posixmodule.c#newcode3607
Modules/posixmodule.c:3607: Py_END_ALLOW_THREADS
On 2012/05/30 21:29:27, storchaka wrote:
> Py_BEGIN_ALLOW_THREADS

Done.  Thanks!

Sign in to reply to this message.

Jim.Jewett

http://bugs.python.org/review/14626/diff/5114/Lib/shutil.py File Lib/shutil.py (right): http://bugs.python.org/review/14626/diff/5114/Lib/shutil.py#newcode185 Lib/shutil.py:185: os.setxattr(dst, attr, os.getxattr(src, attr, follow_symlinks=symlinks), follow_symlinks=symlinks) I kept reading ...

11 months, 1 week ago #3

http://bugs.python.org/review/14626/diff/5114/Lib/shutil.py
File Lib/shutil.py (right):

http://bugs.python.org/review/14626/diff/5114/Lib/shutil.py#newcode185
Lib/shutil.py:185: os.setxattr(dst, attr, os.getxattr(src, attr,
follow_symlinks=symlinks), follow_symlinks=symlinks)
I kept reading this as a cut-and-paste doubling.  It might be more clear if you
broke the lines apart, like either:

    xattr=os.getxattr(src, attr, follow_symlinks=symlinks)
    os.setxattr(dst, xattr, follow_symlinks=symlinks)

or


    attr=os.getxattr(src, attr, follow_symlinks=symlinks)
    os.setxattr(dst, attr, follow_symlinks=symlinks)

depending on how you feel about rebinding loop variables.

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c
File Modules/posixmodule.c (right):

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode620
Modules/posixmodule.c:620: }
Are you just special-casing floats because they coerce to an integer?  They
aren't the only type that does, though they are the most prominent.

Do you want to use the __index__, or PyLongCheck?

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode629
Modules/posixmodule.c:629: if (long_value < INT_MIN) {
Can file descriptors be negative?  If so, is there already a problem with
DEFAULT_DIR_FD of (-100)?  (I do not have an answer for that, other than to
document it.)

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode2235
Modules/posixmodule.c:2235: "stat(path, *, dir_fd=None, fd=None,
follow_symlinks=True) -> stat result\n\n\
Should path have a default of None, to better support fd?

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode2241
Modules/posixmodule.c:2241: the file/directory the file descriptor is open
to.\n\
No other arguments should be specified?  Or does should follow_symlinks be
explicitly set to False?

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode2247
Modules/posixmodule.c:2247: If they are unavailable, using them will raise a
NotImplementedError.");
So will not using follow_symlinks (and therefore getting the default True) cause
a problem?  If so, maybe the name needs to be reversed to
use_symlink_itself=True, or some such.  But I agree that following symlinks is a
better default, so long as you can document that systems without symlink support
treat them as regular files.

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode2551
Modules/posixmodule.c:2551: opened on a directory, not a file.");
Equivalent to chdir(fd=fildes)

Whether to add the "Equivalent to" wording to the existing redundant functions
is a matter of taste; I would prefer to always add it to the docstrings, as it
makes things more clear for those trying to figure out the differences so that
they can decide which to use.

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode3218
Modules/posixmodule.c:3218: If it is unavailable, using it will raise a
NotImplementedError.");
How bad would it bean to just accept an fd in place of a path, and branch on the
type?  It seems more convenient, but I'll gladly yield if you explicitly
rejected the idea.

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode3292
Modules/posixmodule.c:3292: wnamebuf = malloc((len + 5) * sizeof(wchar_t));
At first I read this as "the function might get passed *.*, and need the full 8
chars anyhow."  Maybe "add 5 characters because of the directory marker, the *.*
that gets appended, and the null byte."

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode3945
Modules/posixmodule.c:3945: if ((src_dir_fd != DEFAULT_DIR_FD) || (dst_dir_fd !=
DEFAULT_DIR_FD)) {
I'm not sure how universal my preference is, but I would rather see this test
only once, with the HAVE_RENAMEAT test inside it.

Sign in to reply to this message.

Thanks for the feedback, Jim!  The changes discussed below
will be in my next diff, which (the spirit willing) will also
be after I get the patch working under Windows.

http://bugs.python.org/review/14626/diff/5114/Lib/shutil.py
File Lib/shutil.py (right):

http://bugs.python.org/review/14626/diff/5114/Lib/shutil.py#newcode185
Lib/shutil.py:185: os.setxattr(dst, attr, os.getxattr(src, attr,
follow_symlinks=symlinks), follow_symlinks=symlinks)
I broke it up into two lines; the relevant variables are now
"name" and "value".

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c
File Modules/posixmodule.c (right):

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode620
Modules/posixmodule.c:620: }
On 2012/06/12 03:16:16, Jim.Jewett wrote:
> Are you just special-casing floats because they coerce
> to an integer?

Yes.  I copy-and-pasted this code from the "case 'i'" handler inside
convertsimple() from Python/getargs.c.  That
specific line is me inlining the float_argument_error()
check.  I figured it'd be a safe bet to use the exact
same code as we currently use for signed integer arguments.

Is there some advantage to using PyNumber_Index() here?

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode629
Modules/posixmodule.c:629: if (long_value < INT_MIN) {
I can find no documentation anywhere that says unequivocably
one way or the other.  However: AT_FDCWD on my system is
defined as -100.  I looked it up in POSIX (IEEE Std 1003.1-2008)
and they don't require a particular value for the constant.
Also, classically the "invalid filehandle" value is -1.  I can't
think of any functions where you'd want to pass in -1 for a file
handle argument, but I can't think of any conceptual reason why
it wouldn't be okay.

Long story short, file handles are always signed ints, so
my implementation needs to support negative values.

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode2235
Modules/posixmodule.c:2235: "stat(path, *, dir_fd=None, fd=None,
follow_symlinks=True) -> stat result\n\n\
It already did, I just neglected to fix the docstring.
Fixed!

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode2241
Modules/posixmodule.c:2241: the file/directory the file descriptor is open
to.\n\
If fd is not None, then specifying follow_symlinks=False is
an error.  It's only legal to specify follow_symlinks=True
when fd is used, simply because the default value for an
argument should always be legal.

I've clarified the docstring for stat() slightly in this
regard; it now explicitly states that using fd and
follow_symlinks at the same time is an error.

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode2247
Modules/posixmodule.c:2247: If they are unavailable, using them will raise a
NotImplementedError.");
For the first section, please see previous comment (re: line 2241).

For the last sentence, I'm not sure I understand what you
mean.  You're talking about encountering symlinks on systems
that don't support symlinks?  When would that happen?  In
any case it's getting pretty far out-of-band for poor
stat().  If this is a legitimate problem, well, lstat() has
lived this long without dealing with it, maybe we can live a
little longer.

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode2551
Modules/posixmodule.c:2551: opened on a directory, not a file.");
Done, and I'll follow through on the rest of the
equivalent spots in other docstrings.

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode3218
Modules/posixmodule.c:3218: If it is unavailable, using it will raise a
NotImplementedError.");
I explicitly rejected the idea.  But I can see the attraction.
I've emailed BFDL to see if he cares to yield an opinion.

I'm willing to listen to arguments.  Certainly it would clean
up call sites.

Can you cite any functions in the standard library that behave
in this way?  I don't mean path-versus-fd (though that'd help
a lot if there were any).  I just mean functions that accept
a specific set of types for a parameter.  type() obviously accepts lots of
things but being polymorphic is its whole purpose in life, so I suggest that's a
bad example.

(Parameters that can be either None or something else don't count!)

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode3292
Modules/posixmodule.c:3292: wnamebuf = malloc((len + 5) * sizeof(wchar_t));
Clarified.

http://bugs.python.org/review/14626/diff/5114/Modules/posixmodule.c#newcode3945
Modules/posixmodule.c:3945: if ((src_dir_fd != DEFAULT_DIR_FD) || (dst_dir_fd !=
DEFAULT_DIR_FD)) {
I stuck the result of the calculation in a temporary
("dir_fd_specified").  Since only one of those two uses
will get past the preprocessor, the compiler will optimize
it away anyway.  I think it's a small improvement.

Sign in to reply to this message.

Georg

Hi Larry, most comments are minor, and don't need to be addressed before feature-freeze. But ...

11 months ago #5

larry

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst File Doc/library/os.rst (right): http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode636 Doc/library/os.rst:636: These parameters are used as flags to the \*at ...

11 months ago #6

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst
File Doc/library/os.rst (right):

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode636
Doc/library/os.rst:636: These parameters are used as flags to the \*at family of
functions.
On 2012/06/22 20:26:02, Georg wrote:
> Are there any *at functions anymore?

No.  Good call.  I've removed these five attributes completely (the constants
themselves and the documentation).

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode695
Doc/library/os.rst:695: Equivalent to ``os.chmod(fd, mode)``.
On 2012/06/22 20:26:02, Georg wrote:
> Since it existed, or just as of the patch?  In the
> latter case, that must be noted.  Ditto for a few more
> instances below.

Just as part of the patch.

Happy to note it as you suggest, but not sure what
specifically you have in mind.  Can you give me an example
of how I should note this?  "versionadded" seems inaccurate.

> Hmm, thinking about it more, IMO the f* functions should
> be deprecated (in docs only) right away.

Deprecated how?  If I just say "deprecated" people will think they might start
throwing exceptions.  I doubt we will get rid of fchmod--at least, until Python
4.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode834
Doc/library/os.rst:834: If *dir_fd* is not ``None``, it should be a file
descriptor open to a
On 2012/06/22 20:26:02, Georg wrote:
> Strange wording, maybe better "file descriptor referring to a directory", or
> something like that?

Done.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode835
Doc/library/os.rst:835: directory, and *path* should be relative; path will then
be relative to
On 2012/06/22 20:26:02, Georg wrote:
> What happens if path is absolute?

According to the POSIX 1003.1-2008 page on openat(), if path is absolute, dir_fd
is ignored.  This is the consistent behavior for all *at() functions in POSIX. 
They is also the behavior for all the additional non-POSIX *at() functions
defined in glibc (e.g. fchownat, mkfifoat).

This leads one to theorize: should we detect this situation and instead throw an
exception, chiding the user for mixing up their P's and Q's?  My knee-jerk
reaction: no, let's not make Python overly pedantic.  The POSIX semantics are
good enough for me.

> Also, the second "path" should be "the effective path"
> or something.

No, I genuinely just meant "path" there.  But I guess that's not clear.

However, the "effective path" is, the cumulative result of all transformations
to the path, and is therefore absolute by definition.

You know what I mean to say here; I'd be interested in an alternate suggestion.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode1192
Doc/library/os.rst:1192: If *dir_fd* is not ``None``, it should be a file
descriptor open to a
On 2012/06/22 20:26:02, Georg wrote:
> Same comments as above for this paragraph for "open()".

Done (equivalently to how I responded for "open()").

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode1203
Doc/library/os.rst:1203: unavailable, using it will raise a
:exc:`NotImplementedError`.
On 2012/06/22 20:26:02, Georg wrote:
> Would it be too bad to fall back to use the real ids
> instead of raising?

That would be counterproductive.  If the user says "test this using the
effective ids" they should either get results or know that they didn't.

Consider: if I did the failover as you suggest, you'd never know whether you
actually used the effective ids, or if it fell back to the real ids.

> I assume that Windows is the system without effective ids.
> Does it have an alternate concept?
> 
> (I'm thinking of ease of use of the API.)

TBH I have no idea.  For the implementation here I merged the new function
faccessat() which made no attempt at Windows support.

(Windows doesn't even support read-only directories!)

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode1210
Doc/library/os.rst:1210: using it will raise a :exc:`NotImplementedError`.
On 2012/06/22 20:26:02, Georg wrote:
> Same as above: would it make sense to ignore the arg if follow_symlinks is not
> supported, but the fact is irrelevant because the system doesn't support
> symlinks at all?

What system doesn't support symlinks at all?

Hint: Windows has supported symlinks since at least version 6.0 ("Windows
Vista").  (I thought it supported them earlier than that, but CPython doesn't
seem to think so.)

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode1268
Doc/library/os.rst:1268: Added the *dir_fd*, *effective_ids*, and
*follow_symlinks* parameters.
On 2012/06/22 20:26:02, Georg wrote:
> This paragraph has moved to the wrong location; should be directly below
> "access".

Done.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode1313
Doc/library/os.rst:1313: .. function:: chflags(path, flags, *, follow_symlinks)
On 2012/06/22 20:26:02, Georg wrote:
> =True

Done.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode1460
Doc/library/os.rst:1460: *follow_symlinks* when specifying *path* as an open
file descriptor.
On 2012/06/22 20:26:02, Georg wrote:
> /me wonders if it would make sense to have these canned paragraphs somewhere
as
> footnotes and just link to them.  But this is an improvement for later...

I wondered that too.  Maybe the elaborate explanation can go under
supports_follow_symlinks (et al) and we can just link to that.  But as you say,
later.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode1500
Doc/library/os.rst:1500: file descriptor open to a directory, and the
corresponding path (*src* or *dst*)
On 2012/06/22 20:26:02, Georg wrote:
> again "descriptor open to a directory".  That reads really strange to me.

Done.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode2095
Doc/library/os.rst:2095: A queryable collection of functions, reflecting which
functions in the
On 2012/06/22 20:26:02, Georg wrote:
> "queryable collection" is not a common term.
> Is it a list, a set, ...?

It's a set.  But I was trying to leave it ambiguous.  What I was looking for
here was saying "an object you can use the 'in' operator on."  It's not a map,
and though it is a iterable that's a poor description.

I've changed
"A queriable collection of functions, reflecting" 
to
"An object implementing collections.Set indicating"
Do you have another wording you'd prefer?

(p.s. I considered making it a callable, but I think a set is just nicer.  And
probably faster, if you care about such microbenchmarks.)

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode2109
Doc/library/os.rst:2109: Currently *dir_fd* parameters only work on UNIX and
UNIX-like platforms;
On 2012/06/22 20:26:02, Georg wrote:
> I think we spell it "Unix".

Fun trivia: there is a UNIX certification process.  According to the trademark
holders, only systems that have been thus certified can be called UNIX.  Mac OS
X has been certified; Linux never has.  Therefore Linux cannot be rightly called
UNIX, but only UNIX-like.

But I have dropped the phrase "and UNIX-like" as you request.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode2232
Doc/library/os.rst:2232: name.  Please see the documentation for :func:`unlink`
for
On 2012/06/22 20:26:02, Georg wrote:
> That links back to itself.  Do you mean the manpage?  There's separate markup
> for that.

No, I meant to link to "remove" again.  Done.

http://bugs.python.org/review/14626/diff/5182/Lib/os.py
File Lib/os.py (right):

http://bugs.python.org/review/14626/diff/5182/Lib/os.py#newcode189
Lib/os.py:189: if ((sys.platform != "linux") or
On 2012/06/22 20:26:02, Georg wrote:
> Is "or" correct here?  I understand the comment above such that the function
> exists (so "HAVE_LCHMOD" exists?), so that "and" would be correct.

"or" is correct here.  Sorry it's unclear--I guess my comment isn't helping.

This paragraph is enumerating the os.* functions whose "follow_symlinks"
parameter actually works.  For os.chmod, there are two functions in glibc on
Linux that could possibly enable this functionality: lchmod() and fchmodat().

lchmod() doesn't *actually* work.  It's defined, but when you call it it returns
ENOSUP.  Every. Time.  ./configure correctly detects this and concludes "lchmod
doesn't work".  So HAVE_LCHMOD is actually *not* defined on Linux.

fchmodat() defines a flag that would allow it to support follow_symlinks.  But
the manpage specifically says "note: this flag doesn't work, and specifying it
results in ENOSUP."  ./configure is not sophisticated enough to detect this, and
I'll be damned if I'm going to experiment with it here in os.py.

My theory: There is no working Linux system call for "chmod directly on a
symlink".  If one is added someday, lchmod() will start working, *and* the funny
flag for fchmodat() will work too at the same time.

Hence my solution: on Linux, if lchmod() is not available, ignore the fact that
fchmodat() is available and assume that "follow_symlinks" doesn't work for chmod
at all.

Restating that: if the platform is not linux, or if we have lchmod(), assume
that os.chmod(follow_symlinks) works if we have fchmodat().  Which is what the
code says.

Practically speaking, it's unlikely that a platform exists where the flag for
fchmodat() worked but lchmod() was unavailable.

http://bugs.python.org/review/14626/diff/5182/Lib/shutil.py
File Lib/shutil.py (right):

http://bugs.python.org/review/14626/diff/5182/Lib/shutil.py#newcode146
Lib/shutil.py:146: follow = not(symlinks and os.path.islink(src) and
os.path.islink(dst))
On 2012/06/22 20:26:02, Georg wrote:
> "not" is not a function; should be a space here.

Done.

http://bugs.python.org/review/14626/diff/5182/Lib/shutil.py#newcode162
Lib/shutil.py:162: call("utime")(dst, ns=(st.st_atime_ns, st.st_mtime_ns),
On 2012/06/22 20:26:02, Georg wrote:
> This call will fail with _nop as "utime".
Fixed; added optional keyword parameter "ns" to _nop.  Good catch!

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_os.py
File Lib/test/test_os.py (right):

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_os.py#newcode359
Lib/test/test_os.py:359: def test_lutimes_ns(self):
On 2012/06/22 20:26:02, Georg wrote:
> "lutimes" still in the test function name.  (Hmm, you could argue that the C
> function called is still that one.)

I admit I didn't do as much cleanup here as I could have.

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_os.py#newcode372
Lib/test/test_os.py:372: with self.assertRaises(ValueError):
On 2012/06/22 20:26:02, Georg wrote:
> Why does the exception class change here?

I don't understand the question.

This tests passing in *both* "times" and "ns", which is an error.  IIRC which
exception it should raise was discussed in Rietveld feedback for the utime issue
(#14127).  I think ValueError is the right choice.

Does that answer your question?

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_os.py#newcode1845
Lib/test/test_os.py:1845: kernel_version = platform.release()
On 2012/06/22 20:26:02, Georg wrote:
> Uh, that might match version 2.6.x of some unrelated Unix, right?

I suppose.  But the *xattr functions are most definitely Linux-only.  If you
like I'll throw a platform == linux check in.

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_posix.py
File Lib/test/test_posix.py (right):

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_posix.py#newcode154
Lib/test/test_posix.py:154: @unittest.skipUnless(getattr(os, 'execve') in
os.supports_fd, "test needs execve() to support the fd parameter")
On 2012/06/22 20:26:02, Georg wrote:
> getattr() with no third parameter will still raise AttributeError.

Fixed.  Good catch!

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_posix.py#newcode529
Lib/test/test_posix.py:529: def _test_chflags_regular_file(self, chflags_func,
target_file, kwargs):
On 2012/06/22 20:26:02, Georg wrote:
> Why no **kwargs?

If you look below, I'm passing literal dicts in instead of using keyword
parameters.  I guess that style isn't great--I think I was in a hurry at the
time.

Anyway, changed to use **kwargs so people like you aren't surprised.

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_shutil.py
File Lib/test/test_shutil.py (right):

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_shutil.py#newcode691
Lib/test/test_shutil.py:691: if 0:
On 2012/06/22 20:26:02, Georg wrote:
> This should have a comment why it was disabled.

Oops!  This was debugging scaffolding; I was isolating the last test to make gdb
easier.

Fixed!  Thanks!

http://bugs.python.org/review/14626/diff/5182/Misc/NEWS
File Misc/NEWS (right):

http://bugs.python.org/review/14626/diff/5182/Misc/NEWS#newcode40
Misc/NEWS:40: - Issue #14626: Large refactoring of functions / parameters in os
module.
On 2012/06/22 20:26:02, Georg wrote:
> "in the os module".

Done.

http://bugs.python.org/review/14626/diff/5182/Modules/_io/fileio.c
File Modules/_io/fileio.c (right):

http://bugs.python.org/review/14626/diff/5182/Modules/_io/fileio.c#newcode399
Modules/_io/fileio.c:399: fd_is_own = 1;
On 2012/06/22 20:26:02, Georg wrote:
> Looks unrelated.

Not sure why this is in my diff; I haven't touched this file.  This code is
checked in to trunk.  Before cutting a new diff I will make sure I overwrite my
Modules/_io/fileio.c with a fresh one from trunk.

http://bugs.python.org/review/14626/diff/5182/Modules/posixmodule.c
File Modules/posixmodule.c (right):

http://bugs.python.org/review/14626/diff/5182/Modules/posixmodule.c#newcode7559
Modules/posixmodule.c:7559: #ifdef MS_WINDOWS
On 2012/06/22 20:26:02, Georg wrote:
> tabs here... :)

Done.  Curse you, stock MSVC 2010!

Sign in to reply to this message.

larry

Replying to Serihy's comment which got lost in the mix somehow. http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst File Doc/library/os.rst (right): ...

11 months ago #7

Georg

11 months ago #8

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst
File Doc/library/os.rst (right):

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode695
Doc/library/os.rst:695: Equivalent to ``os.chmod(fd, mode)``.
On 2012/06/23 00:27:56, larry wrote:
> On 2012/06/22 20:26:02, Georg wrote:
> > Since it existed, or just as of the patch?  In the
> > latter case, that must be noted.  Ditto for a few more
> > instances below.
> 
> Just as part of the patch.
> 
> Happy to note it as you suggest, but not sure what
> specifically you have in mind.  Can you give me an example
> of how I should note this?  "versionadded" seems inaccurate.

"From Python 3.3, equivalent to ..."  But I favor the deprecation.

> > Hmm, thinking about it more, IMO the f* functions should
> > be deprecated (in docs only) right away.
> 
> Deprecated how?  If I just say "deprecated" people will think they might start
> throwing exceptions.  I doubt we will get rid of fchmod--at least, until
Python
> 4.

No, that's not how our deprecations work.  Deprecated APIs can, and at some
point should, only emit DeprecationWarnings, not raise exceptions.  You could
even explicitly say they won't be removed before 4.0 using the
"deprecated-removed" directive.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode835
Doc/library/os.rst:835: directory, and *path* should be relative; path will then
be relative to
On 2012/06/23 00:27:56, larry wrote:
> On 2012/06/22 20:26:02, Georg wrote:
> > What happens if path is absolute?
> 
> According to the POSIX 1003.1-2008 page on openat(), if path is absolute,
dir_fd
> is ignored.  This is the consistent behavior for all *at() functions in POSIX.

> They is also the behavior for all the additional non-POSIX *at() functions
> defined in glibc (e.g. fchownat, mkfifoat).
> 
> This leads one to theorize: should we detect this situation and instead throw
an
> exception, chiding the user for mixing up their P's and Q's?  My knee-jerk
> reaction: no, let's not make Python overly pedantic.  The POSIX semantics are
> good enough for me.

Agreed.

> > Also, the second "path" should be "the effective path"
> > or something.
> 
> No, I genuinely just meant "path" there.  But I guess that's not clear.
> 
> However, the "effective path" is, the cumulative result of all transformations
> to the path, and is therefore absolute by definition.
> 
> You know what I mean to say here; I'd be interested in an alternate
suggestion.

Right.  Your version is correct with path marked up as *path*.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode1210
Doc/library/os.rst:1210: using it will raise a :exc:`NotImplementedError`.
On 2012/06/23 00:39:23, larry wrote:
> Serhiy Storchaka had an additional comment here, which Rietveld can't find for
> some reason:
> > And what if the system supports symlink and doesn't
> > support at-functions, follow_symlinks=False and file is
> > not symlink. Should it be NotImplementedError?
> 
> I see what you mean--if the file is not a symlink, then follow_symlinks should
> be irrelevant.  But the NotImplementedError indicates whether or not the
> functionality is available.
> 
> It's equivalent to saying "if I call lchown() on a file which isn't a symbolic
> link, but lchown isn't supported, can't it just call chown for me?"  If lchown
> isn't available, the program won't link / won't run.  If lchown is available,
> but is a non-working stub (as it is on my Linux box), the function returns an
> error rather than calling chown on my behalf.
> 
> It's plausible that Python could be nicer than C here and fall back when
> possible.  Right now I'm just trying to get the patch in at all; if I succeed
in
> that, I'll come back to this.

For now I agree, let "explicit is better than implicit" prevail.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode2095
Doc/library/os.rst:2095: A queryable collection of functions, reflecting which
functions in the
On 2012/06/23 00:27:56, larry wrote:
> On 2012/06/22 20:26:02, Georg wrote:
> > "queryable collection" is not a common term.
> > Is it a list, a set, ...?
> 
> It's a set.  But I was trying to leave it ambiguous.  What I was looking for
> here was saying "an object you can use the 'in' operator on."  It's not a map,
> and though it is a iterable that's a poor description.
> 
> I've changed
> "A queriable collection of functions, reflecting" 
> to
> "An object implementing collections.Set indicating"
> Do you have another wording you'd prefer?
> 
> (p.s. I considered making it a callable, but I think a set is just nicer.  And
> probably faster, if you care about such microbenchmarks.)

You could always say a "set-like object", or just "collection" since the "use
the in operator" part is made quite explicit below.

http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode2109
Doc/library/os.rst:2109: Currently *dir_fd* parameters only work on UNIX and
UNIX-like platforms;
On 2012/06/23 00:27:56, larry wrote:
> On 2012/06/22 20:26:02, Georg wrote:
> > I think we spell it "Unix".
> 
> Fun trivia: there is a UNIX certification process.  According to the trademark
> holders, only systems that have been thus certified can be called UNIX.  Mac
OS
> X has been certified; Linux never has.  Therefore Linux cannot be rightly
called
> UNIX, but only UNIX-like.
> 
> But I have dropped the phrase "and UNIX-like" as you request.

Ah, I wasn't even talking about that, just the casing.  But dropping the
"Unix-like" is also good for consistency.

http://bugs.python.org/review/14626/diff/5182/Lib/os.py
File Lib/os.py (right):

http://bugs.python.org/review/14626/diff/5182/Lib/os.py#newcode189
Lib/os.py:189: if ((sys.platform != "linux") or
On 2012/06/23 00:27:56, larry wrote:
> On 2012/06/22 20:26:02, Georg wrote:
> > Is "or" correct here?  I understand the comment above such that the function
> > exists (so "HAVE_LCHMOD" exists?), so that "and" would be correct.
> 
> "or" is correct here.  Sorry it's unclear--I guess my comment isn't helping.
> 
> This paragraph is enumerating the os.* functions whose "follow_symlinks"
> parameter actually works.  For os.chmod, there are two functions in glibc on
> Linux that could possibly enable this functionality: lchmod() and fchmodat().
> 
> lchmod() doesn't *actually* work.  It's defined, but when you call it it
returns
> ENOSUP.  Every. Time.  ./configure correctly detects this and concludes
"lchmod
> doesn't work".  So HAVE_LCHMOD is actually *not* defined on Linux.

OK, that was the unclear bit.

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_os.py
File Lib/test/test_os.py (right):

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_os.py#newcode372
Lib/test/test_os.py:372: with self.assertRaises(ValueError):
On 2012/06/23 00:27:56, larry wrote:
> On 2012/06/22 20:26:02, Georg wrote:
> > Why does the exception class change here?
> 
> I don't understand the question.
> 
> This tests passing in *both* "times" and "ns", which is an error.  IIRC which
> exception it should raise was discussed in Rietveld feedback for the utime
issue
> (#14127).  I think ValueError is the right choice.
> 
> Does that answer your question?

Yes.  It seems an unrelated change for this patch; therefore I flagged it.

http://bugs.python.org/review/14626/diff/5182/Lib/test/test_os.py#newcode1845
Lib/test/test_os.py:1845: kernel_version = platform.release()
On 2012/06/23 00:27:56, larry wrote:
> On 2012/06/22 20:26:02, Georg wrote:
> > Uh, that might match version 2.6.x of some unrelated Unix, right?
> 
> I suppose.  But the *xattr functions are most definitely Linux-only.  If you
> like I'll throw a platform == linux check in.

Cannot hurt, for self-documenting purposes.

Sign in to reply to this message.

#14626: os module: use keyword-only arguments for dir_fd and nofollow to reduce function count

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Messages