[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/4] os-posix: report error message when lock file failed |
Date: |
Wed, 01 Oct 2014 12:24:55 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
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.