fs: Two fixes for set_file_contents #55
Conversation
This definitely falls under my oft-repeated "move fast and make broken things" mantra 😅. In fairness, I put a |
src/fs.rs
Outdated
| write(&fd, data)?; | ||
| fdatasync(&fd)?; | ||
| ) | ||
| .map(std::fs::File::from)?; |
There was a problem hiding this comment.
Any reason to not just leave the ? on the openat() operation and do let file = File::from(fd); after? I find that a lot more readable than having to know what .map() on an Option<> means.
The variable name here is wrong now. This isn't an fd anymore.
There was a problem hiding this comment.
Also: this is a bit DRY-unfriendly with respect to the block above. I wonder if we should have some kind of a util::write_to_fd() helper that does this pattern of taking an fd, creating a File from it, .write_all() to it, then calling sync. I know this is only strengthening your argument for cap-std... :)
There was a problem hiding this comment.
Another aside: I've been wondering if we should stop calling fdatasync() so much. I've noticed that this is making things really slow. I added cache=unsafe to our VM runs to help a bit, but it's clearly a problem. We probably need to think about this more systematically...
In particular, do we really need to do this with fsverity files? It probably makes sense there to let fsverity do its job, no?
This particular aside isn't actionable for this PR. This code here is used for bootloader resources, so it's probably more valuable here...
There was a problem hiding this comment.
Any reason to not just leave the ? on the openat() operation and do let file = File::from(fd); after?
I think that's a style thing that can go either way, will change it in the next push.
I find that a lot more readable than having to know what .map() on an Option<> means.
This however: map on an Option is a very fundamental Rust thing I think (and it's not a complex API either). You will see nontrivial "combinator" usage out there (and to be fair it's easy to sometimes go too far on combinators) but this isn't at all unusual usage I think.
A basic example is:
let f = File::open("/etc/foo")?;
let f = BufReader::new(f);
vs
let f = File::open("/etc/foo").map(BufReader::new)?;
I use the latter a lot because I think it is just shorter and more readable.
There was a problem hiding this comment.
There's like 5 separate things in this one thread so not marking resolved but I changed the map
There was a problem hiding this comment.
Thanks. The others were more like drive-bys for discussion points not directly related to anything here...
There was a problem hiding this comment.
This however:
mapon an Option is a very fundamental Rust thing I think (and it's not a complex API either). You will see nontrivial "combinator" usage out there (and to be fair it's easy to sometimes go too far on combinators) but this isn't at all unusual usage I think.
Ya. I think this is maybe another case of me not being hip to the jive or whatever the kids these days are doing. I know that I really dislike seeing these.massive.chains().of().linked().operators()......
There was a problem hiding this comment.
Ya. I think this is maybe another case of me not being hip to the jive or whatever the kids these days are doing. I know that I really dislike seeing these.massive.chains().of().linked().operators()......
There are IMO some things that are super elegant as combinators that are straightforward but verbose in explicit loop form, for example the map() + .sum() as used here:
I was just skimming this code and noticed we were incorrectly using `write`. I'd like to reopen that conversation about using cap-std and especially cap-std-ext, which would have fixed several of these bugs with our nice high level helper API like https://docs.rs/cap-std-ext/latest/cap_std_ext/dirext/trait.CapStdExtDirExt.html#tymethod.atomic_replace_with Anyways for now I just fixed the implementation here. - Add a doc comment - Add O_CLOEXEC - Use `write_all` instead of a single `write` (!) - Add a test (Also a different bug, this function takes a `Stat` but ignores everything except st_mode, but we can fix that later) Signed-off-by: Colin Walters <[email protected]>
cgwalters
force-pushed
the
set_file_contents_fixes
branch
from
dc06d92 to
55367de
Compare
I was just skimming this code and noticed
we were incorrectly using
write.I'd like to reopen that conversation about using
cap-std and especially cap-std-ext, which
would have fixed several of these bugs
with our nice high level helper API like
https://docs.rs/cap-std-ext/latest/cap_std_ext/dirext/trait.CapStdExtDirExt.html#tymethod.atomic_replace_with
Anyways for now I just fixed the implementation here.
write_allinstead of a singlewrite(!)(Also a different bug, this function takes a
Statbutignores everything except st_mode, but we can fix that later)