On 2014/10/24 15:32, Michael Tokarev wrote:
> On 09/25/2014 01:46 PM, arei.gong...@huawei.com wrote:
>> 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;
>> }
>
>
> 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
