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

[Gonglei]

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