Allow filepath.FilePath to open files with specific flags

[Ying Li]

Currently, opening a file in filepath.FilePath does not allow you to pass OS flags like read, write, append, truncate, exclusive, etc. Either change open so that it can take said flags, or create a new function that takes said flags.

[Ying Li]

Added an additional optional "flags" parameter to FilePath.open - if it is not None, passes the flags straight to os.open, ignoring mode.

[therve]

• open must return a file handle (you probably want to use os.fdopen). It probably needs a test to check this behavior
• the test should be named test_openWithFlags
• it would be totally awesome if the assert in open could be removed, a meaningful expression raised instead, and a test added. That's not mandatory for this ticket though.

Thanks!

[Ying Li]

• os.fdopen takes a mode, so some additional code generating a mode from the given flags was added (and also changed it so that if both flags and mode are given, an error is raised). This is to prevent the user from inputting incompatible mode and flags (ex. mode='r', flags=os.O_WRONLY) since then os.fdopen will raise a somewhat unhelpful error along the lines of "Invalid argument" or something of the sort. A bunch of tests to make sure that the modes were correctly generated, or at least that behavior is correct for given flags, were added. These tests call read/write on the file handle returned, so that basically also answers the question of whether what's returned is a valid file handle.

• test is renamed, and the other test (which was named testOpen) has been changed to have an underscore. I also named it test_openWithMode to differentiate it from test_openWithFlags

• assert has been removed, and a ValueError raised instead. I'm not sure if this is what you mean, but I did include a test for it in test_openWithMode.

[therve]

• open is documented to raise OSError, but I think you meant ValueError
• test_openWithFlags should be split in 9 or 10 smaller tests: basically, the comments you wrote are almost docstrings for the tests where they should belong.

Thanks!

[Ying Li]

Ok, changed both things. I was just following the style of the original testOpen (now test_openWithMode).

[therve]

(In [23180]) Apply cyli's patch with small cleanups.

Refs #3123

[therve]

(In [23181]) Rename test methods, add a skip.

Refs #3123

[therve]

(In [23190]) Branching to 'filepath-open-flags-3123'

[Jean-Paul Calderone]

What if FilePath didn't try to be as smart here? It's okay for these APIs to be low level (consider, eg FilePath.chown which uses -1 to indicate no-change :). Something to keep in mind is that someday there may be an IFilePath and some of these methods may end up on it, and someone may have to implement them for some other backend. So simplicity in implementation is nice (particularly if the API is amenable to a higher-level API for the common base being implemented on top of it such that it will work with any implementation).

So do you think FilePath.open could require mode to be supplied if flags is and just use what's specified, requiring it to be sensible? I haven't thought about this too much, this is just an initial reaction to the code. I'm open to the possibility that the current approach is better.

[Ying Li]

As far as I can tell, there is only one mapping of flags to mode - it's not really "smart", it just saves the user some work because they'd have to write the same flag-to-mode mapping code if they wanted to use open with flags anyway. That is, unless they're hardcoding in some flags, in which case I guess it would save them a few seconds of thinking and mentally mapping the flags onto a mode. I thought it would be better to provide the convenience rather than make anyone who wanted to open a FilePath with flags to write their own version.

Also, the error returned by os.fdopen when mode and flags don't match is pretty vague - it would make debugging for a user somewhat complicated. If a more helpful error were to be returned, error checking would need to be done using the same code anyway.

If your concern is that there may be an implementation of IFilePath that needs completely different flags and modes... I submitted this patch because I am working on a FilePath based SFTP server, and I need to pass file transfer flags to open. These flags map onto the os file flags perfectly, and as it's a network protocol, it seems that the [read, write, readwrite, append, truncate, create] flags are universal enough. Do you think there should be a separate enumeration of universal flags in FilePath that should be used instead of the ones defined in the os module, so that the flags can be specified for different implementations of IFilePath?

Anyway, I appreciate the comment. :) It didn't seem like a review though, so did you delete the "review" keyword by accident? I notice that you didn't reassign it to me, which is why I ask. I put the review keyword back just in case - apologies if it was intentional.

[Jean-Paul Calderone]

I just dropped the review keyword to make sure I got your attention. I meant to re-assign it to you as well, but forgot. Sorry about that.

I'll think some more about the rest of your response (or maybe someone else will do a review).

[Jean-Paul Calderone]

Prior offline discussion went something like this:

Change the preferred signature of FilePath.open from what it currently is to open(modeObject) where modeObject is an object with getModeString and getModeFlags methods (perhaps with better names). getModeString returns a string like "wb+". getModeFlags returns flags suitable for use with os.open. If the mode string is sufficient to completely describe how to open the file, FilePath.open can just use file and return the result. If the flags are necessary, then os.open is used, along with os.fdopen to create a file object.

The current string-style parameter should remain supported, of course, for backwards compatibility.

[Jean-Paul Calderone]

Also, it occurs to me that most mode strings can be extrapolated from a set of mode flags. However, I don't think that "b" vs "t" can be. It might make sense to drop getModeString in favor of getTextMode so as to minimize the redundancy.

[Glyph]

Replying to exarkun:

Also, it occurs to me that most mode strings can be extrapolated from a set of mode flags.

Yes, but I wonder: is it actually wise to do that? ​http://pubs.opengroup.org/onlinepubs/009695399/functions/fopen.html makes no mention of flags, which makes me wonder if there is some buggy C library that we will need to work around in the future with some bonus non-standard mode string for which there is no equivalent mode flag...

(Obviously the IFilePath version shouldn't care about dumb stuff like that, but maybe FilePath itself should allow the passthrough to be more direct? I don't know.)

However, I don't think that "b" vs "t" can be. It might make sense to drop getModeString in favor of getTextMode so as to minimize the redundancy.

For what it's worth, as per ​http://docs.python.org/library/os.html#open-flag-constants 'b' vs. 't' can be described by O_BINARY vs. O_TEXT (which are only defined on Windows, but then, these flags only have an effect on Windows). But U can't be.

[Glyph]

Replying to exarkun:

Prior offline discussion went something like this:

Change the preferred signature of FilePath.open from what it currently is to open(modeObject) where modeObject is an object with getModeString and getModeFlags methods (perhaps with better names). getModeString returns a string like "wb+". getModeFlags returns flags suitable for use with os.open. If the mode string is sufficient to completely describe how to open the file, FilePath.open can just use file and return the result. If the flags are necessary, then os.open is used, along with os.fdopen to create a file object.

Reflecting on this, I don't like it very much now. Instead of something like this:

from twisted.python.filepath import FilePath
from os import O_RDWR, O_CREAT
# ...
FilePath(foo).open(O_RDWR | O_CREAT)

with this proposal we'd have something like:

from twisted.python.filepath import FilePath, Pointless
from os import O_RDWR, O_CREAT
# ...
FilePath(foo).open(Pointless(O_RDWR | O_CREAT))

Since the benefit of the latter proposal is not actually specified by the comment above, I'm assuming the goal is to make the implementation of FilePath.open shorter (i.e. less smart), but that could be equally well accomplished by just making a modeStringFromFlags function that tidies up the bit-twiddling out of the function itself.

Also, not clearly shown above is that if your function merely expects a FilePath rather than constructing one, you will go from needing zero imports to use a mode string to needing imports from two different modules to use a mode ... object thing.

My current proposal is: FilePath.open(read=True, write=True, create=True, exclusive=True) for the default case.

Then, perhaps a real-filesystem-only escape hatch so you can do platform-specific craziness like FilePath.open(read=True, write=True, create=True, extra=os.O_NOATIME) if you really want to. (Separate ticket.)

That signature will make it a lot easier for e.g. ZipPath to raise a sensible error in the write=True case; it just has to do if write:.

Better yet, I really like the idea of having a bunch of @params that describe how each flag actually works, instead of the conventional 'meh, you know how POSIX works, right' blow-off that both the standard library and our current documentation give.

Unfortunately, this is still slightly inconvenient for the original use-case; you can't pass flags directly from the wire through to os.open. But then, as long as the features requested by the flags required by the standard are exposed by FilePath, perhaps passing them direct from the wire is a bad thing. Wire clients probably shouldn't be able to pass O_DIRECT, for example.

The current string-style parameter should remain supported, of course, for backwards compatibility.

For sure. Let's take care not to make the new, "good" version so much more inconvenient that everybody keeps wanting the compatibility version forever, though.

[Jean-Paul Calderone]

I might not have enough bandwidth to fully consider these most recent comments. However, one thing that did occur to me and which may be a good idea is that instead of FilePath.open(read=True, write=True, create=True, exclusive=True) we could go with the result of #5384 for this instead of a bunch of booleans (maybe this is also ill conceived, but I functions that take sixteen booleans also don't strike me as super rad).