[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/4] os-posix: report error messa
From: |
Gonglei |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed |
Date: |
Wed, 1 Oct 2014 19:58:08 +0800 |
> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
> lock file failed
>
> Gonglei <address@hidden> writes:
>
> > Hi,
> >
> >> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
> >> lock file failed
> >>
> >> <address@hidden> writes:
> >>
> >> > From: Gonglei <address@hidden>
> >> >
> >> > 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 <address@hidden>
> >> > Signed-off-by: Gonglei <address@hidden>
> >> > ---
> >> > 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.
If we can get the root reason of one error easier, why not?
(I encountered the scenario described in commit message,
I only repeated that issue and eventually got the root reason is locking
pid file failed)
I don't think it is a problem that reporting errors over
two or three places.
And I often encounter this coding style:
Int funA() {
...
Printf(".....");
Return -1;
}
Int funcB() {
If (funA() < 0) {
Printf("something wrong\n");
Return -1;
}
}
Best regards,
-Gonglei