[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [PATCH 2/4] os-posix: report error message when lock
From: |
Gonglei |
Subject: |
Re: [Qemu-trivial] [PATCH 2/4] os-posix: report error message when lock file failed |
Date: |
Fri, 24 Oct 2014 16:56:17 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 |
On 2014/10/24 15:32, Michael Tokarev wrote:
> On 09/25/2014 01:46 PM, address@hidden wrote:
>> 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;
>> }
>
>
> I think I'll just revert this patch, because indeed, it makes
> no sense to do this like that.
>
> Context:
>
> qemu_create_pidfile() is only created from main(), and there,
> if that function returns failure, os_pidfile_error() function
> is called, to, guess that, report error (which is done differently
> whenever we're daemonizing or not).
>
> qemu_create_pidfile() function has several error returns, this
> lockf() failure is one of them, there are others (another shown
> in the patch context too).
>
Yes, agree.
> So this patch makes whole thing inconsistent at least.
>
> If we need to show error message when we're daemonizing, it
> looks like we should modify os_pidfile_error() routine to always
> report error and only after that check for daemon mode. This way
> all errors will be reported the same way.
>
In os_pidfile_error(), like below...
if (write(fds[1], &status, 1) != 1) {
perror("daemonize. Writing to pipe\n");
}
will call its child process to report error:
else if (status == 1) {
fprintf(stderr, "Could not acquire pidfile\n");
exit(1);
}
I just want to make the error message more clear so that
people can find the root reasons of errors as soon as possible. :)
> But I really wonder if we actually need os_pidfile_error() in the
> first place, why this can't be done in qemu_create_pidfile().
>
> So, I'm reverting this change now, to be revisited very soon.
>
If you think that's better, I'm fine with it.
Best regards,
-Gonglei