On Thu, Oct 27, 2022 at 12:14:43PM +0200, Fiona Ebner wrote:
> Commit 85c4bf8aa6 ("vl: Unlink absolute PID file path") made it a
> critical error when the PID file path cannot be resolved. Before this
> commit, it was possible to invoke QEMU when the PID file was a file
> created with mkstemp that was already unlinked at the time of the
> invocation. There might be other similar scenarios.
>
> It should not be a critical error when the PID file unlink notifier
> can't be registered, because the path can't be resolved. Turn it into
> a warning instead.
>
> Fixes: 85c4bf8aa6 ("vl: Unlink absolute PID file path")
> Reported-by: Dominik Csapak <d.csa...@proxmox.com>
> Suggested-by: Thomas Lamprecht <t.lampre...@proxmox.com>
> Signed-off-by: Fiona Ebner <f.eb...@proxmox.com>
> ---
>
> For completeness, here is a reproducer based on our actual invocation
> written in Rust (depends on the "nix" crate). It works fine with QEMU
> 7.0, but not anymore with 7.1.
>
> use std::fs::File;
> use std::io::Read;
> use std::os::unix::io::{AsRawFd, FromRawFd};
> use std::path::{Path, PathBuf};
> use std::process::Command;
>
> fn make_tmp_file<P: AsRef<Path>>(path: P) -> (File, PathBuf) {
> let path = path.as_ref();
>
> let mut template = path.to_owned();
> template.set_extension("tmp_XXXXXX");
> match nix::unistd::mkstemp(&template) {
> Ok((fd, path)) => (unsafe { File::from_raw_fd(fd) }, path),
> Err(err) => panic!("mkstemp {:?} failed: {}", template, err),
> }
> }
>
> fn main() -> Result<(), Box<dyn std::error::Error>> {
> let (mut pidfile, pid_path) = make_tmp_file("/tmp/unlinked.pid.tmp");
> nix::unistd::unlink(&pid_path)?;
>
> let mut qemu_cmd = Command::new("./qemu-system-x86_64");
> qemu_cmd.args([
> "-daemonize",
> "-pidfile",
> &format!("/dev/fd/{}", pidfile.as_raw_fd()),
> ]);
>
> let res = qemu_cmd.spawn()?.wait_with_output()?;
>
> if res.status.success() {
> let mut pidstr = String::new();
> pidfile.read_to_string(&mut pidstr)?;
> println!("got PID {}", pidstr);
> } else {
> panic!("QEMU command unsuccessful");
> }
> Ok(())
> }
>
> softmmu/vl.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index b464da25bc..10dfe773a7 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2432,10 +2432,9 @@ static void qemu_maybe_daemonize(const char *pid_file)
>
> pid_file_realpath = g_malloc0(PATH_MAX);
> if (!realpath(pid_file, pid_file_realpath)) {
> - error_report("cannot resolve PID file path: %s: %s",
> - pid_file, strerror(errno));
> - unlink(pid_file);
> - exit(1);
> + warn_report("not removing PID file on exit: cannot resolve PID
> file"
> + " path: %s: %s", pid_file, strerror(errno));
> + return;
> }
I don't think using warn_report is desirable here.
If the behaviour of passing a pre-unlinked pidfile is considered
valid, then we should allow it without printing a warning every
time an application does this.
warnings are to highlight non-fatal mistakes by applications, and
this is not a mistake, it is intentionally supported behaviour.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
