[Qemu-devel] [PATCH 0/4] cleanup -daemonize and pidfile creation a bit

[Qemu-devel] [PATCH 0/4] cleanup -daemonize and pidfile creation a bit

[Michael Tokarev]

The $subject theme is quite messy, with many things done in a cryptic
and twisty way without any need for that.  This trivial patch series
tries to clean up and improve it.  In particular, it aims to make
pidfile errors visible with -daemonize -- something which Gonglei
tried to fix in e5048d15ce6a and which we're revering because that
fix turned out to be incomplete and wrong.

The patches are simple but I split them into several to easy review.

Michael Tokarev (4):
  os-posix: use global daemon_pipe instead of cryptic fds[1]
  os-posix: replace goto again with a proper loop
  pidfile: stop making pidfile error a special case
  os-posix: reorder parent notification for -daemonize

 include/qemu-common.h |    1 -
 os-posix.c            |   62 ++++++++++++++++++-------------------------------
 os-win32.c            |    5 ----
 vl.c                  |    2 +-
 4 files changed, 23 insertions(+), 47 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/4] os-posix: use global daemon_pipe instead of cryptic fds[1]

[Michael Tokarev]

When asked to -daemonize, we fork a child and setup a pipe between
it and parent to pass exit status.  os-posix.c used global fds[2]
array for that, but actually only the writing side of the pipe is
needed to be global, and this name is really too generic.  Use
just one interger for the writing side of the pipe, and name it
daemon_pipe to be more understandable than cryptic fds[1].

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 os-posix.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index e31a099..d687896 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -47,7 +47,7 @@
 static struct passwd *user_pwd;
 static const char *chroot_dir;
 static int daemonize;
-static int fds[2];
+static int daemon_pipe;
 
 void os_setup_early_signal_handling(void)
 {
@@ -205,6 +205,7 @@ void os_daemonize(void)
 {
     if (daemonize) {
         pid_t pid;
+        int fds[2];
 
         if (pipe(fds) == -1) {
             exit(1);
@@ -236,7 +237,8 @@ void os_daemonize(void)
             }
 
         close(fds[0]);
-        qemu_set_cloexec(fds[1]);
+        daemon_pipe = fds[1];
+        qemu_set_cloexec(daemon_pipe);
 
         setsid();
 
@@ -263,7 +265,7 @@ void os_setup_post(void)
         ssize_t len;
 
     again1:
-        len = write(fds[1], &status, 1);
+        len = write(daemon_pipe, &status, 1);
         if (len == -1 && (errno == EINTR)) {
             goto again1;
         }
@@ -296,7 +298,7 @@ void os_pidfile_error(void)
 {
     if (daemonize) {
         uint8_t status = 1;
-        if (write(fds[1], &status, 1) != 1) {
+        if (write(daemon_pipe, &status, 1) != 1) {
             perror("daemonize. Writing to pipe\n");
         }
     } else
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/4] os-posix: use global daemon_pipe instead of cryptic fds[1]

[Gonglei]

On 2014/10/30 23:07, Michael Tokarev wrote:

> When asked to -daemonize, we fork a child and setup a pipe between
> it and parent to pass exit status.  os-posix.c used global fds[2]
> array for that, but actually only the writing side of the pipe is
> needed to be global, and this name is really too generic.  Use
> just one interger for the writing side of the pipe, and name it
> daemon_pipe to be more understandable than cryptic fds[1].
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  os-posix.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Best regards,
-Gonglei

[Qemu-devel] [PATCH 2/4] os-posix: replace goto again with a proper loop

[Michael Tokarev]

Elminiate two fullwrite implementations with goto replacing them with
a proper do..while loop.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 os-posix.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index d687896..eada8d4 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -218,11 +218,9 @@ void os_daemonize(void)
 
             close(fds[1]);
 
-        again:
-            len = read(fds[0], &status, 1);
-            if (len == -1 && (errno == EINTR)) {
-                goto again;
-            }
+            do {
+                len = read(fds[0], &status, 1);
+            } while (len < 0 && errno == EINTR);
             if (len != 1) {
                 exit(1);
             }
@@ -264,11 +262,9 @@ void os_setup_post(void)
         uint8_t status = 0;
         ssize_t len;
 
-    again1:
-        len = write(daemon_pipe, &status, 1);
-        if (len == -1 && (errno == EINTR)) {
-            goto again1;
-        }
+        do {        
+            len = write(daemon_pipe, &status, 1);
+        } while (len < 0 && errno == EINTR);
         if (len != 1) {
             exit(1);
         }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/4] os-posix: replace goto again with a proper loop

[Gonglei]

On 2014/10/30 23:07, Michael Tokarev wrote:

> Elminiate two fullwrite implementations with goto replacing them with

s/Elminiate/Eliminate/

> a proper do..while loop.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  os-posix.c |   16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)

Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Best regards,
-Gonglei

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

[Michael Tokarev]

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

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 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);
-            }
-            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);
+
+        } else if (pid < 0) {
+            exit(1);
+        }
 
         close(fds[0]);
         daemon_pipe = fds[1];
@@ -290,17 +284,6 @@ void os_setup_post(void)
     }
 }
 
-void os_pidfile_error(void)
-{
-    if (daemonize) {
-        uint8_t status = 1;
-        if (write(daemon_pipe, &status, 1) != 1) {
-            perror("daemonize. Writing to pipe\n");
-        }
-    } else
-        fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
-}
-
 void os_set_line_buffering(void)
 {
     setvbuf(stdout, NULL, _IOLBF, 0);
diff --git a/os-win32.c b/os-win32.c
index 5f95caa..c0daf8e 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -104,11 +104,6 @@ void os_parse_cmd_args(int index, const char *optarg)
     return;
 }
 
-void os_pidfile_error(void)
-{
-    fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
-}
-
 int qemu_create_pidfile(const char *filename)
 {
     char buffer[128];
diff --git a/vl.c b/vl.c
index f6b3546..150524c 100644
--- a/vl.c
+++ b/vl.c
@@ -3999,7 +3999,7 @@ int main(int argc, char **argv, char **envp)
 #endif
 
     if (pid_file && qemu_create_pidfile(pid_file) != 0) {
-        os_pidfile_error();
+        fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
         exit(1);
     }
 
-- 
1.7.10.4

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

[Gonglei]

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/

> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  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?

> -            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);
> +
> +        } else if (pid < 0) {
> +            exit(1);
> +        }
>  
>          close(fds[0]);
>          daemon_pipe = fds[1];
> @@ -290,17 +284,6 @@ void os_setup_post(void)
>      }
>  }
>  
> -void os_pidfile_error(void)
> -{
> -    if (daemonize) {
> -        uint8_t status = 1;
> -        if (write(daemon_pipe, &status, 1) != 1) {
> -            perror("daemonize. Writing to pipe\n");
> -        }
> -    } else
> -        fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
> -}
> -
>  void os_set_line_buffering(void)
>  {
>      setvbuf(stdout, NULL, _IOLBF, 0);
> diff --git a/os-win32.c b/os-win32.c
> index 5f95caa..c0daf8e 100644
> --- a/os-win32.c
> +++ b/os-win32.c
> @@ -104,11 +104,6 @@ void os_parse_cmd_args(int index, const char *optarg)
>      return;
>  }
>  
> -void os_pidfile_error(void)
> -{
> -    fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
> -}
> -
>  int qemu_create_pidfile(const char *filename)
>  {
>      char buffer[128];
> diff --git a/vl.c b/vl.c
> index f6b3546..150524c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3999,7 +3999,7 @@ int main(int argc, char **argv, char **envp)
>  #endif
>  
>      if (pid_file && qemu_create_pidfile(pid_file) != 0) {
> -        os_pidfile_error();
> +        fprintf(stderr, "Could not acquire pid file: %s\n", strerror(errno));
>          exit(1);
>      }
>  

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 <mjt@tls.msk.ru>
>> ---
>>  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

Re: [Qemu-devel] [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 <mjt@tls.msk.ru>
>>> ---
>>>  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
> 

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

[Michael Tokarev]

31.10.2014 10:33, Gonglei wrote:
[]
>>>> 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'?

Damn.  I missed that one.  It is "indentation" inedeed :)
(but not identification).

[]
>>>>              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?

with len != 1, the condition 'len == 1 && status == 0 ? 0 : 1'
evaluates to 1.  Maybe I can add a comment here:

+            /* only exit successfully if our child actually
+             * wrote a one-byte zero to our pipe */
+            exit(len == 1 && status == 0 ? 0 : 1);

See the result at http://git.corpit.ru/?p=qemu.git;a=shortlog;h=refs/heads/trivial-patches-next

Thanks,

/mjt

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

[Gonglei]

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

> 31.10.2014 10:33, Gonglei wrote:
> []
>>>>> 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'?
> 
> Damn.  I missed that one.  It is "indentation" inedeed :)
> (but not identification).
> 
> []
>>>>>              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?
> 
> with len != 1, the condition 'len == 1 && status == 0 ? 0 : 1'

Ok. That's will be great if you can modify it
to "(len == 1 && status == 0 ) ? 0 : 1"

Best regards,
-Gonglei

> evaluates to 1.  Maybe I can add a comment here:
> 

> +            /* only exit successfully if our child actually
> +             * wrote a one-byte zero to our pipe */
> +            exit(len == 1 && status == 0 ? 0 : 1);
> 
> See the result at http://git.corpit.ru/?p=qemu.git;a=shortlog;h=refs/heads/trivial-patches-next
> 
> Thanks,
> 
> /mjt

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

[Michael Tokarev]

31.10.2014 10:58, Gonglei wrote:
> On 2014/10/31 15:41, Michael Tokarev wrote:
[]
>>>>>> +            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?
>>
>> with len != 1, the condition 'len == 1 && status == 0 ? 0 : 1'
> 
> Ok. That's will be great if you can modify it
> to "(len == 1 && status == 0 ) ? 0 : 1"

Well, ? operator has lowest precedence in C, && and || is higher,
and == is even higher.  That's the basic rules of the language.
So I don't really see why... ;)

The comment however should clear all confusion, hopefully.

>> evaluates to 1.  Maybe I can add a comment here:
>>
> 
>> +            /* only exit successfully if our child actually
>> +             * wrote a one-byte zero to our pipe */
>> +            exit(len == 1 && status == 0 ? 0 : 1);
>>
>> See the result at http://git.corpit.ru/?p=qemu.git;a=shortlog;h=refs/heads/trivial-patches-next

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

[Gonglei]

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

> 31.10.2014 10:58, Gonglei wrote:
>> On 2014/10/31 15:41, Michael Tokarev wrote:
> []
>>>>>>> +            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?
>>>
>>> with len != 1, the condition 'len == 1 && status == 0 ? 0 : 1'
>>
>> Ok. That's will be great if you can modify it
>> to "(len == 1 && status == 0 ) ? 0 : 1"
> 
> Well, ? operator has lowest precedence in C, && and || is higher,
> and == is even higher.  That's the basic rules of the language.
> So I don't really see why... ;)
> 
> The comment however should clear all confusion, hopefully.
> 

Hmm...

BTW, Do you receive mails subscribed form Qemu community?
Does the Qemu maillist system crash?
Since yesterday evening, my colleagues and I do not received mails.
It's too weird!

Thanks!
-Gonglei

[Qemu-devel] [PATCH 4/4] os-posix: reorder parent notification for -daemonize

[Michael Tokarev]

Put "success" parent reporting in os_setup_post() to after
all other initializers which may also fail, to the very end,
so more possible failure cases are reported properly to the
calling process.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 os-posix.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index a3b96d9..9f4a9b3 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -253,15 +253,6 @@ void os_setup_post(void)
     int fd = 0;
 
     if (daemonize) {
-        uint8_t status = 0;
-        ssize_t len;
-
-        do {        
-            len = write(daemon_pipe, &status, 1);
-        } while (len < 0 && errno == EINTR);
-        if (len != 1) {
-            exit(1);
-        }
         if (chdir("/")) {
             perror("not able to chdir to /");
             exit(1);
@@ -276,11 +267,21 @@ void os_setup_post(void)
     change_process_uid();
 
     if (daemonize) {
+        uint8_t status = 0;
+        ssize_t len;
+
         dup2(fd, 0);
         dup2(fd, 1);
         dup2(fd, 2);
 
         close(fd);
+
+        do {        
+            len = write(daemon_pipe, &status, 1);
+        } while (len < 0 && errno == EINTR);
+        if (len != 1) {
+            exit(1);
+        }
     }
 }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/4] os-posix: reorder parent notification for -daemonize

[Gonglei]

On 2014/10/30 23:07, Michael Tokarev wrote:

> Put "success" parent reporting in os_setup_post() to after
> all other initializers which may also fail, to the very end,
> so more possible failure cases are reported properly to the
> calling process.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  os-posix.c |   19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Best regards,
-Gonglei