cleanup -daemonize and pidfile creation a bit

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 <[email protected]>

---
 os-posix.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Elminiate two fullwrite implementations with goto replacing them with

a proper do..while loop.

Signed-off-by Michael Tokarev <[email protected]>

---
 os-posix.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

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 <[email protected]>

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

include/qemu-common.h +0 -1

index b87e9c2..f862214 100644
@@ -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)

os-posix.c +0 -0

index eada8d4..a3b96d9 100644

os-posix.c +6 -20

-            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);

os-win32.c +0 -5

index 5f95caa..c0daf8e 100644
@@ -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];

vl.c +1 -1

index f6b3546..150524c 100644
@@ -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);
     }
 

Quoted 7 lines · 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().

Quoted 6 lines

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/

Quoted 18 lines

Signed-off-by Michael Tokarev <[email protected]>

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

include/qemu-common.h +0 -1

index b87e9c2..f862214 100644
@@ -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);
 

os-posix.c void os_daemonize(void) · line 226

Quoted 13 lines

include/qemu-common.h +0 -0

 /* Convert a byte between binary and BCD.  */
 static inline uint8_t to_bcd(uint8_t val)

os-posix.c +0 -3

index eada8d4..a3b96d9 100644
@@ -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?

os-posix.c void os_daemonize(void) · line 225

Quoted 12 lines

os-posix.c +2 -9

-            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);

os-posix.c void os_daemonize(void) · line 230

Quoted 5 lines

os-posix.c +4 -0

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

Quoted 14 lines · Gonglei

Quoted 12 lines · 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.

Quoted 3 lines

Quoted 1 line

While at it, fix wrong identation in os_daemonize().

s/identation/identification/

No, the original word was the right one.

Quoted 18 lines

Quoted 18 lines

Signed-off-by Michael Tokarev <[email protected]>

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

include/qemu-common.h +0 -1

index b87e9c2..f862214 100644
@@ -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);
 

Quoted 15 lines

os-posix.c void os_daemonize(void) · line 226

Quoted 13 lines

include/qemu-common.h +0 -0

 /* Convert a byte between binary and BCD.  */
 static inline uint8_t to_bcd(uint8_t val)

os-posix.c +0 -3

index eada8d4..a3b96d9 100644
@@ -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...

Quoted 12 lines

os-posix.c void os_daemonize(void) · line 225

Quoted 12 lines

os-posix.c +2 -9

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

Quoted 5 lines

os-posix.c void os_daemonize(void) · line 230

Quoted 5 lines

os-posix.c +4 -0

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

Thanks,

/mjt

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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'?

Quoted 23 lines

Quoted 8 lines

os-posix.c void os_daemonize(void) · line 226

Quoted 6 lines

             do {
                 len = read(fds[0], &status, 1);
             } while (len < 0 && errno == EINTR);

os-posix.c +0 -3

-            if (len != 1) {
-                exit(1);
-            }

Does this check need to be removed?

Yes, because...

Quoted 12 lines

Quoted 12 lines

os-posix.c +2 -9

-            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);

Quoted 1 line

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

31.10.2014 10:33, Gonglei wrote:
[]

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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

[]

Quoted 23 lines

Quoted 23 lines

Quoted 8 lines

os-posix.c void os_daemonize(void) · line 226

Quoted 6 lines

             do {
                 len = read(fds[0], &status, 1);
             } while (len < 0 && errno == EINTR);

os-posix.c +0 -3

-            if (len != 1) {
-                exit(1);
-            }

Does this check need to be removed?

Yes, because...

Quoted 12 lines

Quoted 12 lines

os-posix.c +2 -9

-            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);

Quoted 4 lines

Quoted 1 line

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

(quoted diff) +3 -0

+            /* 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

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

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

+ 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"

Quoted 8 lines

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

(quoted diff) +3 -0

+            /* 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

Quoted 1 line · Gonglei

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

[]

Quoted 9 lines

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

+ 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'

Quoted 2 lines

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.

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 <[email protected]>

---
 os-posix.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)