| Message ID | 20221027101443.118049-1-f.ebner@proxmox.com |
|---|---|
| State | New |
| Headers | show |
| Series | vl: change PID file path resolve error to warning | expand |
On 27.10.22 12:14, 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> > --- Reviewed-by: Hanna Reitz <hreitz@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
Am 27.10.22 um 14:17 schrieb Daniel P. Berrangé: > On Thu, Oct 27, 2022 at 12:14:43PM +0200, Fiona Ebner wrote: >> + 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 But what if the path resolution fails in a scenario where the caller did not pre-unlik the PID file? Should the warning only be printed when the errno is not ENOENT? Might still not be accurate in all cases though. Best Regards, Fiona
On 28/10/2022 09:11, Fiona Ebner wrote: > Am 27.10.22 um 14:17 schrieb Daniel P. Berrangé: >> On Thu, Oct 27, 2022 at 12:14:43PM +0200, Fiona Ebner wrote: >>> + 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. > > But what if the path resolution fails in a scenario where the caller did > not pre-unlik the PID file? Should the warning only be printed when the > errno is not ENOENT? Might still not be accurate in all cases though. ENOENT would be IMO a good heuristic for silence, as I see no point in warning that something won't be cleaned up if it's already gone from POV of QEMU. Iff, I'd then personally only log at some level that shows up on debug/high-verbosity settings, that should cover the a bit odd setups where, e.g. the PID file is there but not visible to QEMU, e.g., because being located in another mount namespace (in which case the management stack that put it there probably wants to handle that more explicitly anyway). Or do you meant something else with "not accurate in all cases"? best regards, Thomas
Am 28.10.22 um 09:26 schrieb Thomas Lamprecht: > On 28/10/2022 09:11, Fiona Ebner wrote: >> Am 27.10.22 um 14:17 schrieb Daniel P. Berrangé: >>> On Thu, Oct 27, 2022 at 12:14:43PM +0200, Fiona Ebner wrote: >>>> + 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. >> >> But what if the path resolution fails in a scenario where the caller did >> not pre-unlik the PID file? Should the warning only be printed when the >> errno is not ENOENT? Might still not be accurate in all cases though. > > ENOENT would be IMO a good heuristic for silence, as I see no point in > warning that something won't be cleaned up if it's already gone from > POV of QEMU. > > Iff, I'd then personally only log at some level that shows up on > debug/high-verbosity settings, that should cover the a bit odd setups where, > e.g. the PID file is there but not visible to QEMU, e.g., because being > located in another mount namespace (in which case the management stack that > put it there probably wants to handle that more explicitly anyway). Or do > you meant something else with "not accurate in all cases"? > I did not have a concrete scenario in mind, just felt like there could be some edge case where we get ENOENT even though the file is not unlinked or maybe we get EACCES and can't tell if the file is already gone or not. Not sure what would happen in the scenario you described, but it might be one such edge case :) Best Regards, Fiona
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; } qemu_unlink_pidfile_notifier = (struct UnlinkPidfileNotifier) {
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(-)