Re: [PATCH] vl: change PID file path resolve error to warning

["Daniel P. Berrangé" <berrange@redhat.com>]

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.csapak@proxmox.com>
> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@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 :|