Re: [Qemu-trivial] [PATCH 3/4] pidfile: stop making pidfile error a special case

[Gonglei]

On 2014/10/31 15:16, Michael Tokarev wrote:

> 31.10.2014 08:00, Gonglei wrote:
>> On 2014/10/30 23:07, Michael Tokarev wrote:
>>
>>> In case of -daemonize, we write non-zero to the daemon
>>> pipe only if pidfile creation failed, so the parent will
>>> report error about pidfile problem.  There's no need to
>>> make special case for this, since all other errors are
>>> reported by the child just fine.  Let the parent report
>>> error and simplify logic in os_daemonize().
>>>
>>> This way, we don't need os_pidfile_error() function, since
>>> it only prints error now, so put the error reporting printf
>>> into the only place where qemu_create_pidfile() is called,
>>> in vl.c.
>>>
>>> While at it, fix wrong identation in os_daemonize().
>>
>> s/identation/identification/
> 
> No, the original word was the right one.
> 

Sorry, I can't find 'identation' both dictionary and Google.
Your meaning 'indentation'?

>>> Signed-off-by: Michael Tokarev <address@hidden>
>>> ---
>>>  include/qemu-common.h |    1 -
>>>  os-posix.c            |   29 ++++++-----------------------
>>>  os-win32.c            |    5 -----
>>>  vl.c                  |    2 +-
>>>  4 files changed, 7 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>>> index b87e9c2..f862214 100644
>>> --- a/include/qemu-common.h
>>> +++ b/include/qemu-common.h
>>> @@ -357,7 +357,6 @@ char *qemu_find_file(int type, const char *name);
>>>  void os_setup_early_signal_handling(void);
>>>  char *os_find_datadir(void);
>>>  void os_parse_cmd_args(int index, const char *optarg);
>>> -void os_pidfile_error(void);
>>>  
>>>  /* Convert a byte between binary and BCD.  */
>>>  static inline uint8_t to_bcd(uint8_t val)
>>> diff --git a/os-posix.c b/os-posix.c
>>> index eada8d4..a3b96d9 100644
>>> --- a/os-posix.c
>>> +++ b/os-posix.c
>>> @@ -221,18 +221,12 @@ void os_daemonize(void)
>>>              do {
>>>                  len = read(fds[0], &status, 1);
>>>              } while (len < 0 && errno == EINTR);
>>> -            if (len != 1) {
>>> -                exit(1);
>>> -            }
>>
>> Does this check need to be removed?
> 
> Yes, because...
>>
>>> -            else if (status == 1) {
>>> -                fprintf(stderr, "Could not acquire pidfile\n");
>>> -                exit(1);
>>> -            } else {
>>> -                exit(0);
>>> -            }
>>> -            } else if (pid < 0) {
>>> -                exit(1);
>>> -            }
>>> +
>>> +            exit(len == 1 && status == 0 ? 0 : 1);
> 
> ...it is checked here, note the 'len == 1' part of the condition.
> 

If len != 1, the original code exit with 1, after your changes,
it will exit with 0. Right?

> And here comes the wrong original identation -- this part of "else"
> was indented once too many:
> 

I know your meaning now. :)

>>> +
>>> +        } else if (pid < 0) {
>>> +            exit(1);
>>> +        }
>>>  
> 
> Thanks,
> 
> /mjt
>