Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed

[Gonglei]

> 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