Add transitional message #348
Conversation
smoelius
force-pushed
the
transition
branch
5 times, most recently
from
6acf255 to
f15968d
Compare
afl/build.rs
Outdated
| if installing && !building_cargo_afl { | ||
| println!("cargo:warning=You appear to be installing `cargo-afl` with:"); | ||
| println!("cargo:warning= cargo install afl"); | ||
| println!("cargo:warning=A future version of afl.rs will require you to use:"); | ||
| println!("cargo:warning= cargo install cargo-afl"); | ||
| println!("cargo:warning=You can use the new command now, if you like."); | ||
| } |
There was a problem hiding this comment.
Does it make sense to also suggest cargo uninstall afl, or is the latter cargo-afl going to effectively clobber the install anyways?
There was a problem hiding this comment.
Rather than suggest to uninstall, I propose the following. Change the first line to:
warning: You appear to be installing the `cargo-afl` binary with:
And add the following line at the end:
warning: Note: If the binary is already installed, you may need to add --force.
So the revised message would look like:
warning: You appear to be installing the `cargo-afl` binary with:
warning: cargo install afl
warning: A future version of afl.rs will require you to use:
warning: cargo install cargo-afl
warning: You can use the new command now, if you like.
warning: Note: If the binary is already installed, you may need to add --force.
What do you think?
| #[cfg(unix)] | ||
| let _file = sys::lock_path(&work_dir).unwrap(); |
There was a problem hiding this comment.
Just making sure I understand: this doesn't require an associated flock(..., LOCK_UN) because you're expecting the advisory lock to be dropped when the underlying fd is closed at process exit, right?
I believe that should be fine, but it's not super well defined by POSIX AFAICT. Just something to be aware of should it cause unusual behavior on non-Linux builds in the future 🙂
There was a problem hiding this comment.
Just making sure I understand: this doesn't require an associated
flock(..., LOCK_UN)because you're expecting the advisory lock to be dropped when the underlying fd is closed at process exit, right?
That's correct.
I believe that should be fine, but it's not super well defined by POSIX AFAICT. Just something to be aware of should it cause unusual behavior on non-Linux builds in the future 🙂
I'm curious as to what makes you say that (not saying you're wrong).
I'm reading this: https://man7.org/linux/man-pages/man2/flock.2.html
Furthermore, the lock is released
either by an explicit LOCK_UN operation on any of these duplicate
file descriptors, or when all such file descriptors have been
closed.
Is file descriptor closure not well defined?
There was a problem hiding this comment.
Is file descriptor closure not well defined?
I thought POSIX was ambiguous on whether process termination is actually required to close all FDs, but it looks like it's unambiguous:
All of the file descriptors, directory streams, conversion descriptors, and message catalog descriptors open in the calling process shall be closed.
(From https://pubs.opengroup.org/onlinepubs/007904875/functions/exit.html)
There's some subtlety around child processes that inherit the open FD, but I don't think that case matters here. So this is safe to ignore 🙂
|
Thanks a lot for the review, @woodruffw. 🙏 |
This PR implements a transitional message as described in #335.
Essentially, users should now be able to install the
cargo-aflbinary with either of the following commands:However, using the old command now produces the following warning:
This PR also resolves #330.