Opened 14 years ago

Last modified 10 years ago

#3123 enhancement new

Allow filepath.FilePath to open files with specific flags

Reported by: Ying Li Owned by: Ying Li
Priority: normal Milestone:
Component: core Keywords:
Cc: therve, Jean-Paul Calderone Branch: branches/filepath-open-flags-3123
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

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.

Attachments (3)

filepath.open.flags.patch (2.0 KB) - added by Ying Li 14 years ago.
filepath.open.flags.2.patch (6.7 KB) - added by Ying Li 14 years ago.
filepath.open.flags.3.patch (7.7 KB) - added by Ying Li 14 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 14 years ago by Ying Li

Keywords: review added
Owner: Ying Li deleted
Priority: normalhighest

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

Changed 14 years ago by Ying Li

Attachment: filepath.open.flags.patch added

comment:2 Changed 14 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to Ying Li
Priority: highesthigh
  • 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!

comment:3 Changed 14 years ago by Ying Li

Keywords: review added
Owner: Ying Li deleted
Priority: highhighest
  • 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.

Changed 14 years ago by Ying Li

Attachment: filepath.open.flags.2.patch added

comment:4 Changed 14 years ago by therve

Keywords: review removed
Owner: set to Ying Li
  • 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!

comment:5 Changed 14 years ago by Ying Li

Keywords: review added
Owner: Ying Li deleted

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

Changed 14 years ago by Ying Li

Attachment: filepath.open.flags.3.patch added

comment:6 Changed 14 years ago by therve

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

Refs #3123

comment:7 Changed 14 years ago by therve

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

Refs #3123

comment:8 Changed 14 years ago by therve

Owner: set to therve
Priority: highesthigh

comment:9 Changed 14 years ago by therve

author: therve
Branch: branches/filepath-open-flags-3123

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

comment:10 Changed 14 years ago by Jean-Paul Calderone

Keywords: review removed

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.

comment:11 in reply to:  10 Changed 14 years ago by Ying Li

Keywords: review added

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.

comment:12 Changed 14 years ago by 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).

comment:13 Changed 14 years ago by therve

Owner: changed from therve to Jean-Paul Calderone

comment:14 Changed 14 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: changed from Jean-Paul Calderone to Ying Li
Priority: highnormal

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.

comment:15 Changed 14 years ago by 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.

comment:16 Changed 11 years ago by <automation>

Owner: Ying Li deleted

comment:17 Changed 11 years ago by Glyph

Owner: set to Glyph
Status: newassigned

comment:18 Changed 11 years ago by Glyph

Owner: changed from Glyph to Ying Li
Status: assignednew

comment:19 in reply to:  15 Changed 10 years ago by 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.

comment:20 in reply to:  14 Changed 10 years ago by 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.

comment:21 Changed 10 years ago by 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).

Note: See TracTickets for help on using tickets.