Gonglei <arei.gong...@hotmail.com> writes: > Hi, > >> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when >> lock file failed >> >> <arei.gong...@huawei.com> writes: >> >> > From: Gonglei <arei.gong...@huawei.com> >> > >> > It will cause that create vm failed When manager >> > tool is killed forcibly (kill -9 libvirtd_pid), >> > the file not was unlink, and unlock. It's better >> > that report the error message for users. >> > >> > Signed-off-by: Huangweidong <weidong.hu...@huawei.com> >> > Signed-off-by: Gonglei <arei.gong...@huawei.com> >> > --- >> > os-posix.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/os-posix.c b/os-posix.c >> > index 9d5ae70..89831dc 100644 >> > --- a/os-posix.c >> > +++ b/os-posix.c >> > @@ -316,6 +316,7 @@ int qemu_create_pidfile(const char *filename) >> > return -1; >> > } >> > if (lockf(fd, F_TLOCK, 0) == -1) { >> > + error_report("lock file '%s' failed: %s", filename, >> > strerror(errno)); >> > close(fd); >> > return -1; >> > } >> >> Only called from main(): >> > Yes, indeed. > >> if (pid_file && qemu_create_pidfile(pid_file) != 0) { >> os_pidfile_error(); >> exit(1); >> } >> >> I suspect the error reporting is os_pidfile_error()'s job. And it >> actually does it (POSIX version shown, W32 is simpler): >> >> void os_pidfile_error(void) >> { >> if (daemonize) { >> uint8_t status = 1; >> if (write(fds[1], &status, 1) != 1) { >> perror("daemonize. Writing to pipe\n"); >> } >> } else >> fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno)); >> } >> >> Are you sure your patch makes sense? > > Yes, I'm sure it make sense. And I had tested, the result is OK as expected. > > When daemonize variable is true, the os_pidfile_error() usually don't > report an error, > because "if (write(fds[1], &status, 1) != 1)" is always false. On the other > hand, we should assure that we can get the original error message > when lock failed but not other information, such as "Could not acquire > pid file...".
Even if the new error message makes sense, reporting errors in two places doesn't. qemu_create_pidfile() is designed to leave the actual error reporting to its caller, which delegates it to os_pidfile_error(). If daemonize, os_pidfile_error() signals the failure to the parent process instead of reporting to stderr. The parent does the reporting, in os_daemonize(). If this isn't good enough, you're certainly welcome to improve it. Just noticed that commit e5048d1 "os-posix: report error message when lock file failed" already messed this up: error reporting is spread over three places: qemu_create_pidfile(), os_pidfile_error() and os_daemonize(). Please pick one method to report errors: either just in qemu_create_pidfile(), or in os_pidfile_error() (normal case) and os_daemonize() (if daemonize). I don't particularly care which one you pick, as long as it works with and without -daemonize.