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

[Markus Armbruster]

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.