Skip to content

tail: Do not panic in path.is_tailable() check, but return false instead #3919

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.

Sign up for GitHub

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

Closed
Joining7943 wants to merge 2 commits into from
Closed

tail: Do not panic in path.is_tailable() check, but return false instead #3919

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28da3f3
tail: Do not panic in path.is_tailable() check, but return false instead
Joining7943 Sep 9, 2022
c7fede8
tail: Add debugging output when running into todo() and fix metadata
Joining7943 Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/uu/tail/src/tail.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,12 +451,22 @@ fn uu_tail(mut settings: Settings) -> UResult<()> {
}
} else {
// TODO: [2021-10; jhscheer] how to handle block device or socket?

// we're running into this todo() on macos sometimes accidentally, so here's some debug output
// to help identify the problem
dbg!(settings.paths);
dbg!(&path);
dbg!(path.to_string_lossy().to_string());
dbg!(path.is_file());
dbg!(path.exists());
dbg!(path.is_stdin());
dbg!(path_is_tailable);
dbg!(settings.stdin_is_pipe_or_fifo);
dbg!(display_name);
todo!();
}
}

let metadata = path.metadata().ok();

if display_name.is_stdin() && !path.is_file() {
if settings.verbose {
files.print_header(&display_name, !first_header);
Expand Down Expand Up @@ -496,6 +506,7 @@ fn uu_tail(mut settings: Settings) -> UResult<()> {
} else if path_is_tailable {
match File::open(&path) {
Ok(mut file) => {
let metadata = path.metadata().ok();
if settings.verbose {
files.print_header(&display_name, !first_header);
first_header = false;
Expand Down Expand Up @@ -544,6 +555,9 @@ fn uu_tail(mut settings: Settings) -> UResult<()> {
} else {
path.to_owned()
};

let metadata = path.metadata().ok(); // TODO isn't metadata always None here?

// Insert non-is_tailable() paths into `files.map`
files.insert(
&path,
Expand Down Expand Up @@ -1650,8 +1664,9 @@ impl PathExtTail for Path {

/// Return true if `path` is is a file type that can be tailed
fn is_tailable(&self) -> bool {
self.is_file() || self.exists() && self.metadata().unwrap().is_tailable()
self.is_file() || self.exists() && self.metadata().map_or(false, |meta| meta.is_tailable())
Copy link
Copy Markdown
Contributor

@jhscheer jhscheer Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to metadata() will return an error if user lacks permissions or the path does not exist. However, is_file() and exists() already return false if user cannot access metadata. So this unwrap should never be triggered.

Copy link
Copy Markdown
Contributor Author

@Joining7943 Joining7943 Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's what happened

Copy link
Copy Markdown
Contributor

@jhscheer jhscheer Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then there could be an issue with time-of-check / time-of-use. You could try try_exists() instead of exists(), but it's only available in rust 1.63

Copy link
Copy Markdown
Contributor Author

@Joining7943 Joining7943 Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to stick to unwrap()?

Copy link
Copy Markdown
Contributor

@jhscheer jhscheer Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silencing this error doesn't feel right.
This unwrap triggers, but (according to documentation) it shouldn't, which means there's an issue somewhere up in the call stack.

Copy link
Copy Markdown
Contributor Author

@Joining7943 Joining7943 Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about silencing the error but instead returning such a path as untailable, that's what the signature and name of the method states. No surprises.

Copy link
Copy Markdown
Contributor

@niyaznigmatullin niyaznigmatullin Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just

fn is_tailable(&self) -> bool {
    self.metadata().map_or(false, |meta| meta.is_tailable())
}

Copy link
Copy Markdown
Contributor Author

@Joining7943 Joining7943 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

}

/// Workaround to handle redirects, e.g. `touch f && tail -f - < f`
fn handle_redirect(&self) -> PathBuf {
if cfg!(unix) && self.is_stdin() {
Expand Down