New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document and expand the open options #1252

Merged
merged 6 commits into from Nov 23, 2015

Conversation

Projects
None yet
9 participants
@pitdicker
Contributor

pitdicker commented Aug 13, 2015

The options that can be passed to the os when opening a file vary between
systems. And even if the options seem the same or similar, there may be
unexpected corner cases.

This RFC attempts to

  • describe the different corner cases and behaviour of various operating
    systems.
  • describe the intended behaviour and interaction of Rusts options.
  • remedy cross-platform inconsistencies.
  • suggest extra options to expose more platform-specific options.

rendered

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Aug 13, 2015

Member

@pitdicker Thanks so much for taking this on; it's clearly a huge amount of work! I'm excited to dig into the details and hope to leave feedback soon.

Member

aturon commented Aug 13, 2015

@pitdicker Thanks so much for taking this on; it's clearly a huge amount of work! I'm excited to dig into the details and hope to leave feedback soon.

Show outdated Hide outdated 0000-open-options.md
### Create
`.create(true)`
Open an existing file, or create a new file if it already exists.

This comment has been minimized.

@nagisa

nagisa Aug 13, 2015

Contributor

s/if it already exist/if it doesn’t exist/?

@nagisa

nagisa Aug 13, 2015

Contributor

s/if it already exist/if it doesn’t exist/?

Show outdated Hide outdated 0000-open-options.md
support mandatory file locking, but it is just as broken as advisory.
For Rust, the sharing mode can be set with a Windows-specific option. Given the
problems above, i don't expect there to ever be a cross-platform option for file

This comment has been minimized.

@nagisa

nagisa Aug 13, 2015

Contributor

In near future, perhaps not, but I’m pretty sure a way to do it would get implemented in a timely manner should a reason to do so arise.

@nagisa

nagisa Aug 13, 2015

Contributor

In near future, perhaps not, but I’m pretty sure a way to do it would get implemented in a timely manner should a reason to do so arise.

Show outdated Hide outdated 0000-open-options.md
.cache_hint(enum CacheHint)
enum CacheHint {
Normal,

This comment has been minimized.

@nagisa

nagisa Aug 13, 2015

Contributor

Bikeshed: What’s a “normal” cache hint? Perhaps None would be better?

@nagisa

nagisa Aug 13, 2015

Contributor

Bikeshed: What’s a “normal” cache hint? Perhaps None would be better?

@pitdicker

This comment has been minimized.

Show comment
Hide comment
@pitdicker

pitdicker Aug 17, 2015

Contributor

Sorry for making some more commits to the RFC, I am clearly an amateur...
I mainly found some more corner cases on Windows that have to do with .truncate(), and a lot of typos.
But this should be it.

Contributor

pitdicker commented Aug 17, 2015

Sorry for making some more commits to the RFC, I am clearly an amateur...
I mainly found some more corner cases on Windows that have to do with .truncate(), and a lot of typos.
But this should be it.

@alexcrichton alexcrichton self-assigned this Aug 19, 2015

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Aug 25, 2015

Contributor

Just a note about an issue I ran into today. Opening a path to a directory as a file has different results on different platforms, and should be documented also. I was only able to test UNIX, but the results on UNIX were:

  • Opening with read(true) and/or append(true) returned an Ok(File). Setting other options returned os error 21 (is a directory).
  • Calling metadata on that file successfully returned the metadata for that directory.
  • Calling the other file methods caused the program to abort on bad syscall.
  • Calling read() on a directory opened in read/append mode returned error 21 (is a directory).
  • Calling write() on a directory opened in read/append mode returned error 9 (bad file descriptor).
Contributor

withoutboats commented Aug 25, 2015

Just a note about an issue I ran into today. Opening a path to a directory as a file has different results on different platforms, and should be documented also. I was only able to test UNIX, but the results on UNIX were:

  • Opening with read(true) and/or append(true) returned an Ok(File). Setting other options returned os error 21 (is a directory).
  • Calling metadata on that file successfully returned the metadata for that directory.
  • Calling the other file methods caused the program to abort on bad syscall.
  • Calling read() on a directory opened in read/append mode returned error 21 (is a directory).
  • Calling write() on a directory opened in read/append mode returned error 9 (bad file descriptor).
@ehiggs

This comment has been minimized.

Show comment
Hide comment
@ehiggs

ehiggs Aug 26, 2015

The documentation here is pretty extensive. Is it worth mentioning how create_new is the file option option for creating PID files on Unix? This is a really important option for systems programming which isn't available yet.

Also, wouldn't it be better to yield an error if people start mixing flags in broken ways? create(true).truncate(true).create_new(true) will just ignore create and truncate? Seems like a bug if they were all provided together.

ehiggs commented Aug 26, 2015

The documentation here is pretty extensive. Is it worth mentioning how create_new is the file option option for creating PID files on Unix? This is a really important option for systems programming which isn't available yet.

Also, wouldn't it be better to yield an error if people start mixing flags in broken ways? create(true).truncate(true).create_new(true) will just ignore create and truncate? Seems like a bug if they were all provided together.

@pitdicker

This comment has been minimized.

Show comment
Hide comment
@pitdicker

pitdicker Aug 27, 2015

Contributor

@withoutboats, @ehiggs thanks for your comments!

@withoutboats
The combination op open() whith something that is not a file, is quite a hole in this rfc. Thanks for pointing it out.

@ehiggs
I feel a bit difficult for making .create(true).truncate(true).create_new(true) yield an error, as this RCF already indroduces serveral errors while there were none before :(.
So any extra errors should have a good reason.
In this case:
.create(true) and .create_new(true) can not both work together. One has to take precedence over the other, or this should return an error.
.create_new(true) and .truncate(true) are nonsensical. You can not open an existing file, but if you do, truncate it...

In this case I think it is best to return an error if .create_new(true) is combined with .create(true), instead of having the meaning of the code depend on a precendence rule in the documentation. And if we already disallow one combination, then also just disallow the nonsensical combination with .truncate(true) also.

Contributor

pitdicker commented Aug 27, 2015

@withoutboats, @ehiggs thanks for your comments!

@withoutboats
The combination op open() whith something that is not a file, is quite a hole in this rfc. Thanks for pointing it out.

@ehiggs
I feel a bit difficult for making .create(true).truncate(true).create_new(true) yield an error, as this RCF already indroduces serveral errors while there were none before :(.
So any extra errors should have a good reason.
In this case:
.create(true) and .create_new(true) can not both work together. One has to take precedence over the other, or this should return an error.
.create_new(true) and .truncate(true) are nonsensical. You can not open an existing file, but if you do, truncate it...

In this case I think it is best to return an error if .create_new(true) is combined with .create(true), instead of having the meaning of the code depend on a precendence rule in the documentation. And if we already disallow one combination, then also just disallow the nonsensical combination with .truncate(true) also.

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Aug 27, 2015

Contributor

.create_new(true) and .truncate(true) are nonsensical. You can not open an existing file, but if you do, truncate it...

IMO it can be thought about in a way that makes sense:

Open a file only if it does not exist (the create_new part). Truncate it (which is a no-op).

The fact that truncate is a no-op does not make this combination nonsensical at all. I think OpenOptions should always try to avoid erroring and try to do something intuitive.

Contributor

nagisa commented Aug 27, 2015

.create_new(true) and .truncate(true) are nonsensical. You can not open an existing file, but if you do, truncate it...

IMO it can be thought about in a way that makes sense:

Open a file only if it does not exist (the create_new part). Truncate it (which is a no-op).

The fact that truncate is a no-op does not make this combination nonsensical at all. I think OpenOptions should always try to avoid erroring and try to do something intuitive.

@pitdicker

This comment has been minimized.

Show comment
Hide comment
@pitdicker

pitdicker Aug 27, 2015

Contributor

@nagisa Yes, .create_new(true) and .truncate(true) can be ok, but what do you think about the combination of .create_new(true) and .create(true)?

Contributor

pitdicker commented Aug 27, 2015

@nagisa Yes, .create_new(true) and .truncate(true) can be ok, but what do you think about the combination of .create_new(true) and .create(true)?

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 4, 2015

Contributor

It need not be an error, but could be a lint. I'll open an issue over at clippy for now.

Contributor

llogiq commented Sep 4, 2015

It need not be an error, but could be a lint. I'll open an issue over at clippy for now.

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Oct 30, 2015

Contributor

Would be nice to somehow advance it since the discussion has stalled. Could it be possible to include into schedule of the next libs team meeting/do a FCP?

Contributor

nagisa commented Oct 30, 2015

Would be nice to somehow advance it since the discussion has stalled. Could it be possible to include into schedule of the next libs team meeting/do a FCP?

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa

nagisa Oct 30, 2015

Contributor

Yes, .create_new(true) and .truncate(true) can be ok, but what do you think about the combination of .create_new(true) and .create(true)?

I realised I never answered your question. I fear I’ve forgotten my original point. Here’s the current thoughts (that may or may not differ from whatever I said earlier):

In case where two options are mutually exclusive, it is always possible to encode that in the API by removing the possible intersection point. In case of create and create_new the intersection point would be (true, true) which could be reified by doing something along the lines of:

enum CreateType { Create, CreateNew, NopeNopeNope };
fn create(…, type: CreateType)…

However, since we are bound by stability guarantees, I guess simply panicking in the builder finalizer (the open method) is fine.

Contributor

nagisa commented Oct 30, 2015

Yes, .create_new(true) and .truncate(true) can be ok, but what do you think about the combination of .create_new(true) and .create(true)?

I realised I never answered your question. I fear I’ve forgotten my original point. Here’s the current thoughts (that may or may not differ from whatever I said earlier):

In case where two options are mutually exclusive, it is always possible to encode that in the API by removing the possible intersection point. In case of create and create_new the intersection point would be (true, true) which could be reified by doing something along the lines of:

enum CreateType { Create, CreateNew, NopeNopeNope };
fn create(…, type: CreateType)…

However, since we are bound by stability guarantees, I guess simply panicking in the builder finalizer (the open method) is fine.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Oct 30, 2015

Contributor

Just a heads-up: clippy now contains a nonsensical_open_option lint.

Contributor

llogiq commented Oct 30, 2015

Just a heads-up: clippy now contains a nonsensical_open_option lint.

@dhardy

This comment has been minimized.

Show comment
Hide comment
@dhardy

dhardy Oct 31, 2015

Contributor

I don't see why the exact conflicting-options-behaviour (supercede, one takes priority...) matters as long as it is well documented.

Anyway, looks like an amazing API (from my semi-ignorant viewpoint). I'd offer to help with impl/testing (Linux) but it looks like you're way ahead of me.

Contributor

dhardy commented Oct 31, 2015

I don't see why the exact conflicting-options-behaviour (supercede, one takes priority...) matters as long as it is well documented.

Anyway, looks like an amazing API (from my semi-ignorant viewpoint). I'd offer to help with impl/testing (Linux) but it looks like you're way ahead of me.

Show outdated Hide outdated 0000-open-options.md
`.share_mode(DENY_READ | DENY_WRITE | DENY_DELETE)`.
## Caching behaviour

This comment has been minimized.

@alexcrichton

alexcrichton Nov 3, 2015

Member

Could you elaborate a bit on the inclusion of these new options? I would personally expect this to fall under the category of "out of scope for now" in the sense that this should all in theory be possible to implement externally (e.g. using the as-raw-os-handle style traits).

@alexcrichton

alexcrichton Nov 3, 2015

Member

Could you elaborate a bit on the inclusion of these new options? I would personally expect this to fall under the category of "out of scope for now" in the sense that this should all in theory be possible to implement externally (e.g. using the as-raw-os-handle style traits).

Show outdated Hide outdated 0000-open-options.md
os | since version | oldest supported version
:-------------|:--------------|:------------------------
OS X | 10.6 | 10.7?
Linux | 2.6.23 | 2.6.32 (supported by Rust)

This comment has been minimized.

@alexcrichton

alexcrichton Nov 3, 2015

Member

Ah unfortunate we actually support back to 2.6.18, but older kernels just ignore O_CLOEXEC so we just pass it unconditionally and then also set cloexec after opening. This is redundant work on modern Unixes, but gets the job done at least!

@alexcrichton

alexcrichton Nov 3, 2015

Member

Ah unfortunate we actually support back to 2.6.18, but older kernels just ignore O_CLOEXEC so we just pass it unconditionally and then also set cloexec after opening. This is redundant work on modern Unixes, but gets the job done at least!

This comment has been minimized.

@pitdicker

pitdicker Nov 3, 2015

Contributor

At the time I write this part, O_CLOEXEC was not yet passed, but now it is not really an interesting part of the rfc anymore :)

@pitdicker

pitdicker Nov 3, 2015

Contributor

At the time I write this part, O_CLOEXEC was not yet passed, but now it is not really an interesting part of the rfc anymore :)

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 3, 2015

Member

Sorry for taking so look to get around to this @pitdicker, but it all looks great to me! It'd be nice to have a concise overview of the OS-specific extension traits in terms of what the APIs would look like, but I think it's also relatively easy to infer to it's not critical.

I agree with @nagisa and I'll try to bring this up soon at a libs team meeting to move into FCP.

Member

alexcrichton commented Nov 3, 2015

Sorry for taking so look to get around to this @pitdicker, but it all looks great to me! It'd be nice to have a concise overview of the OS-specific extension traits in terms of what the APIs would look like, but I think it's also relatively easy to infer to it's not critical.

I agree with @nagisa and I'll try to bring this up soon at a libs team meeting to move into FCP.

@pitdicker

This comment has been minimized.

Show comment
Hide comment
@pitdicker

pitdicker Nov 3, 2015

Contributor

Everyone thanks for the comments and pursuing. Sorry for not doing this
myself, I got a bit distracted...

I have changed my opinion about 'no access mode set'. It does not really have
any use, and I think it is best to not allow it for now (conservative).

Also, I have rewritten the caching part of the rfc, put not yet pushed.
When I look at it now, it may be better to remove a some parts of the rfc.
What do you think, should the parts about file locking and caching be removed?

Contributor

pitdicker commented Nov 3, 2015

Everyone thanks for the comments and pursuing. Sorry for not doing this
myself, I got a bit distracted...

I have changed my opinion about 'no access mode set'. It does not really have
any use, and I think it is best to not allow it for now (conservative).

Also, I have rewritten the caching part of the rfc, put not yet pushed.
When I look at it now, it may be better to remove a some parts of the rfc.
What do you think, should the parts about file locking and caching be removed?

@pitdicker

This comment has been minimized.

Show comment
Hide comment
@pitdicker

pitdicker Nov 3, 2015

Contributor

@llogiq I keep being amazed how much you can lint for! Very nice to see a nonsensical_open_option lint.

Contributor

pitdicker commented Nov 3, 2015

@llogiq I keep being amazed how much you can lint for! Very nice to see a nonsensical_open_option lint.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 3, 2015

Member

I'd be fine with no access mode == error, I think we'd definitely be able to expand that in the future to return success if an interesting case came up. I'd also be ok leaving the caching/file locking to a separate RFC, although ensuring there's design space for it is fine to mention (although with a builder-style thing we should be pretty set)

Member

alexcrichton commented Nov 3, 2015

I'd be fine with no access mode == error, I think we'd definitely be able to expand that in the future to return success if an interesting case came up. I'd also be ok leaving the caching/file locking to a separate RFC, although ensuring there's design space for it is fine to mention (although with a builder-style thing we should be pretty set)

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Nov 3, 2015

Contributor

@pitdicker We haven't even made a dent in the surface of what can be linted. The nonsensical_open_options lint is actually quite simple.

Btw. if you see other APIs that have ambiguous or confusing API items, just file a clippy issue, no matter if you think it possible to lint or not.

Contributor

llogiq commented Nov 3, 2015

@pitdicker We haven't even made a dent in the surface of what can be linted. The nonsensical_open_options lint is actually quite simple.

Btw. if you see other APIs that have ambiguous or confusing API items, just file a clippy issue, no matter if you think it possible to lint or not.

@dhardy

This comment has been minimized.

Show comment
Hide comment
@dhardy

dhardy Nov 3, 2015

Contributor

@pitdicker I have no clue who would make use of the Windows-specific share mode. It replaces one set of problems with another, though I guess it would be nice to know writes wouldn't collide (if this could be done cross-platform).

Speaking of which, the atomic append operations appear to be limited in length. See here and specifically here. Anywhere from 256 bytes to 4k. Makes it useless for what I wanted to do, but anyway.

Contributor

dhardy commented Nov 3, 2015

@pitdicker I have no clue who would make use of the Windows-specific share mode. It replaces one set of problems with another, though I guess it would be nice to know writes wouldn't collide (if this could be done cross-platform).

Speaking of which, the atomic append operations appear to be limited in length. See here and specifically here. Anywhere from 256 bytes to 4k. Makes it useless for what I wanted to do, but anyway.

@pitdicker

This comment has been minimized.

Show comment
Hide comment
@pitdicker

pitdicker Nov 3, 2015

Contributor

@dhardy I did some testing for the length of atomic append because of these posts, and did not find a limit. I think they found a limit because that language uses buffered IO or a pipe with a buffer.
That is why I put in the rfc to write all data in one operation, or "use a buffered writer with a more than adequately sized buffer".

Contributor

pitdicker commented Nov 3, 2015

@dhardy I did some testing for the length of atomic append because of these posts, and did not find a limit. I think they found a limit because that language uses buffered IO or a pipe with a buffer.
That is why I put in the rfc to write all data in one operation, or "use a buffered writer with a more than adequately sized buffer".

@dhardy

This comment has been minimized.

Show comment
Hide comment
@dhardy

dhardy Nov 3, 2015

Contributor

@pitdicker great news. Sounds like it should be tested on a few different platforms still.

Contributor

dhardy commented Nov 3, 2015

@pitdicker great news. Sounds like it should be tested on a few different platforms still.

@ehiggs

This comment has been minimized.

Show comment
Hide comment
@ehiggs

ehiggs Nov 6, 2015

@llogiq, @Pyriphlegethon: that's great news about the lint. My concern about nonsensical flags are basically solved. Thanks!

ehiggs commented Nov 6, 2015

@llogiq, @Pyriphlegethon: that's great news about the lint. My concern about nonsensical flags are basically solved. Thanks!

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Nov 6, 2015

Contributor

Well, in a perfect world, a lint wouldn't be needed, but when using the type system isn't possible (in this case because of backwards-compatibility), it's the best we can do.

Contributor

llogiq commented Nov 6, 2015

Well, in a perfect world, a lint wouldn't be needed, but when using the type system isn't possible (in this case because of backwards-compatibility), it's the best we can do.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 12, 2015

Member

🔔 This RFC is now entering its week-long final comment period 🔔

Member

alexcrichton commented Nov 12, 2015

🔔 This RFC is now entering its week-long final comment period 🔔

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 19, 2015

Member

The libs team discussed this during triage today and the decision was to merge. @pitdicker would you be ok moving the sections about caching and such to alternatives as well? We were thinking that for now we may not want to dive into those domains but it's certainly possible in the future!

Member

alexcrichton commented Nov 19, 2015

The libs team discussed this during triage today and the decision was to merge. @pitdicker would you be ok moving the sections about caching and such to alternatives as well? We were thinking that for now we may not want to dive into those domains but it's certainly possible in the future!

@pitdicker

This comment has been minimized.

Show comment
Hide comment
@pitdicker

pitdicker Nov 22, 2015

Contributor

@alexcrichton Great news!
I have moved the sections about caching and file locking to the alternatives. Let me know if this is what you had in mind.

Contributor

pitdicker commented Nov 22, 2015

@alexcrichton Great news!
I have moved the sections about caching and file locking to the alternatives. Let me know if this is what you had in mind.

@pitdicker

This comment has been minimized.

Show comment
Hide comment
@pitdicker

pitdicker Nov 22, 2015

Contributor

I have a local branch with most of the changes proposed by this rfc, but it is messy and I don't have time to clean it up before the Christmas holidays. If anyone wants to implement it, let me know. Maybe you can use it as a starting point...

Contributor

pitdicker commented Nov 22, 2015

I have a local branch with most of the changes proposed by this rfc, but it is messy and I don't have time to clean it up before the Christmas holidays. If anyone wants to implement it, let me know. Maybe you can use it as a starting point...

@alexcrichton alexcrichton referenced this pull request Nov 23, 2015

Closed

Tracking issue for OpenOptions expansion #30014

1 of 3 tasks complete

@alexcrichton alexcrichton merged commit b6b4b4b into rust-lang:master Nov 23, 2015

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 23, 2015

Member

Looks good to me, thanks @pitdicker! Feel free to send the PR whenever, and thanks so much again for the RFC!

Member

alexcrichton commented Nov 23, 2015

Looks good to me, thanks @pitdicker! Feel free to send the PR whenever, and thanks so much again for the RFC!

@pitdicker pitdicker deleted the pitdicker:openoptions branch Feb 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment