cp: require preserve only certain attributes #4099
Conversation
f43fc85 to
41df37c
Compare
|
GNU testsuite comparison: |
41df37c to
9f57a09
Compare
a7a13f5 to
00bdcba
Compare
|
GNU testsuite comparison: |
3b88b8a to
3aa2877
Compare
3aa2877 to
a445094
Compare
a445094 to
04678c1
Compare
There was a problem hiding this comment.
So, I think it'd be good to rethink how we handle attributes, cause I don't the approach with the Vec<Attribute> is great. It feels more complex that it needs to be. Instead of an Attribute enum, I think we need an Attributes struct with a Preserve enum. Let me give a sketch:
struct Attributes {
ownership: Preserve,
mode: Preserve,
timestamps: Preserve,
context: Preserve,
links: Preserve,
xattr: Preserve,
}
enum Preserve {
No, // don't preserve
Try, // preserve but don't error
Require, // return error if it doesn't work
}Alternatively:
enum Preserve {
No,
Yes { required: bool },
}The default would be No unless the attribute is specified to be preserved. In the case of "all", we could then do:
impl Attributes {
fn all() -> Self {
Self {
ownership: Preserve::Require,
mode: Preserve::Require,
timestamps: Preserve::Require,
links: Preserve::Require,
context: Preserve::Try,
xattr: Preserve::Try,
}
}
}I think this makes the code much easier to follow.
@tertsdiepraam Nice idea, makes sense :) I wonder what the pub enum Attribute {
#[cfg(unix)]
Ownership { required: bool },
Mode { required: bool },
Timestamps { required: bool },
#[cfg(feature = "feat_selinux")]
Context { required: bool },
Links { required: bool },
Xattr { required: bool },
}Although, this would cause a lot of change to unrelated code, and some unnecessary required setting... |
|
I don't see how the required flag would help with not iterating through each attribute? I agree though that this would change too many things at the same time. |
Ah, I was going the opposite direction here. Was concerned that replacing that iteration over attributes vector with handling each separately would be a negative. I'll try with the struct, let's see how it works out. Thanks! |
|
Oh I only see now that you wrote
I actually want to replace the struct Options {
...
attributes: Attributes,
...
}so fn ignore_if_try<E>(p: Preserve, res: Result<(), E>) -> Result<(),E> {
match res {
Err(e) if p == Preserve::Require => Err(e)
_ => (),
}
}
fn copy_attributes(attrs: Attributes, ...) -> Result<...> {
if attrs.ownership != Preserve::No {
let error = {...};
ignore_if_try(attr.ownership, error)?
}
if attrs.mode != Preserve::No {
...
}
}(I made the |
935066b to
fa3af7a
Compare
|
@tertsdiepraam pushed my version of it :) |
0f90522 to
ff789e8
Compare
|
fails on mac because of: could you have please a look? |
Hm, it looks like the modified time is copied in tests but not when running it locally... Is there some unusual file handling in the tests? GNU cp: uutils/coreutils: |
|
Here's what these files look like during the test: The access time is updates in the test, unlike in the manual run. Maybe difference between how |
ff789e8 to
fca82d1
Compare
# Conflicts: # src/uu/cp/src/copydir.rs # src/uu/cp/src/cp.rs
On Android, this cp with explicit preserve of xattr must fail, because of the limitations of the filesystem setup used on Android.
# Conflicts: # tests/by-util/test_cp.rs
# Conflicts: # src/uu/cp/src/cp.rs
Access timestamps appear to be modified only in this test. Running the command directly does not alter the access timestamp.
This reverts commit d130cf0.
28e7697 to
013bb05
Compare
|
Excellent, thanks! |
In GNU cp, there's a lot of special handling of errors based on using
-a,-p,--preserve=all,--preserve=xattr,contextetc. which is not handled properly at the moment. In this PR, an analogous behavior to that ofrequire_preserve_xattr,require_preserve_context, andrequire_preserveflags in GNU cp code is implemented.Somethings left todo are testing xattr permission denied case (outside of Android test), and also context permission denied case. One idea is making a simple user-space fuse FS with advanced xattr modification permission controls. This will be implemented in a separate PR here - #4100.
Original PR: #4095 (started implemeting testfs there, but it's a bit out of scope, so, I'm separating these PR's).
Closes #4079.