Gonglei <arei.gong...@hotmail.com> writes:
>> Subject: Re: [Qemu-devel] [PATCH 2/4] os-posix: report error message when
>> lock file failed
>>
>> 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.
>
> 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.
