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

[Markus Armbruster <armbru@redhat.com>]

Gonglei <arei.gonglei@hotmail.com> writes:

>> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
>> lock file failed
>> 
>> Gonglei <arei.gonglei@hotmail.com> writes:
>> 
>> > Hi,
>> >
>> >> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
>> >> lock file failed
>> >>
>> >> <arei.gonglei@huawei.com> writes:
>> >>
>> >> > From: Gonglei <arei.gonglei@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.huang@huawei.com>
>> >> > Signed-off-by: Gonglei <arei.gonglei@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.
>
> 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;
>     }
> }

Just because you see this often doesn't make it good code.  It produces
two error messages rather than one.  One of them is usually useless.