ls: force fetching metadata when called with -L -Z#4960
ls: force fetching metadata when called with -L -Z#4960cakebaker merged 2 commits intouutils:mainfrom
Conversation
tertsdiepraam
left a comment
tertsdiepraam
left a comment
There was a problem hiding this comment.
I like the test and this the solution is good. I just think it should be somewhere else, namely in the get_security_context function. That function triggers the error if selinux is enabled so it should probably also trigger the error if it doesn't.
src/uu/ls/src/ls.rs
Outdated
| for item in items { | ||
| let context_len = item.security_context.len(); | ||
| longest_context_len = context_len.max(longest_context_len); | ||
| item.md(out); |
There was a problem hiding this comment.
I think a comment explaining why this is here would be nice. I'm also thinking maybe it makes more sense to fix this in the get_security_context function?
There was a problem hiding this comment.
I've looked at inserting something similar into get_security_context() but I really wanted to use the md() function from PathData as this is the "code path" that is taken in the case of ls -L -l and which generates the error.
At a first glance the patch looked a bit more complex to implement it inside get_security_context() as I don't have direct access to a PathData() or the out variable from get_security_context().
I can try to make it happen with a bit of rework though :)
There was a problem hiding this comment.
Just attempting to dereference in the get_security_context function should work right?
"code path" that is taken in the case of ls -L -l
That's true, but this issue is about a different code path. The error in this path comes from get_security_context and retrieving the metadata is not necessary.
There was a problem hiding this comment.
Essentially what I'm proposing is this:
- If security context is supported and the feature enabled: get the context and return the result
- If not and
must_dereferenceis true:- perform a deref, if it fails, return that error
- return Ok("?".into())
- Else just return Ok("?".into())
There was a problem hiding this comment.
I've updated the code to match your expectations a bit more closely, what about this version now?
|
GNU testsuite comparison: |
granquet
force-pushed
the
ls_LZ
branch
2 times, most recently
from
b413558 to
eaf55c8
Compare
|
GNU testsuite comparison: |
tertsdiepraam
left a comment
tertsdiepraam
left a comment
There was a problem hiding this comment.
Code looks good! I just think it needs a comment to explain why we do this. Feel free to mention me when it's done and I'll take another look :)
|
GNU testsuite comparison: |
The metadata are not used but it permits to check the symlink is valid. We then return 1 on invalid symlinks when ls is invoked with ls -L -Z Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
|
GNU testsuite comparison: |
tertsdiepraam
left a comment
tertsdiepraam
left a comment
There was a problem hiding this comment.
Thanks! It's all good now as soon as the CI is green.
|
GNU testsuite comparison: |
|
Thanks for your PR :) |
3 participants
The metadata are not used but it permits to use the same path as the long format to ensure we return 1 on invalid symlinks when ls is invoked with ls -L -Z
should solve #2928