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

[Michael Tokarev]

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.

>> 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.

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

>> +
>> +        } else if (pid < 0) {
>> +            exit(1);
>> +        }
>>  

Thanks,

/mjt