| Message ID | 20230417225857.2006561-2-bugaevc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | O_IGNORE_CTTY everywhere | expand |
On 17/04/23 19:58, Sergey Bugaev via Libc-alpha wrote: > This is nicer, and is going to be required for the following changes > to reasonably stay within the 79 column limit. > > No functional change. > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> LGTM, some minor nits below. > --- > misc/daemon.c | 88 ++++++++++++++++++++++++++++----------------------- > 1 file changed, 49 insertions(+), 39 deletions(-) > > diff --git a/misc/daemon.c b/misc/daemon.c > index 3c73ac2a..61da49b7 100644 > --- a/misc/daemon.c > +++ b/misc/daemon.c > @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c 8.1 (Berkeley) 6/4/93"; > int > daemon (int nochdir, int noclose) > { > - int fd; > + int fd; > > - switch (__fork()) { > - case -1: > - return (-1); > - case 0: > - break; > - default: > - _exit(0); > - } > + switch (__fork ()) > + { > + case -1: > + return -1; > > - if (__setsid() == -1) > - return (-1); > + case 0: > + break; > > - if (!nochdir) > - (void)__chdir("/"); > + default: > + _exit (0); > + } > > - if (!noclose) { > - struct __stat64_t64 st; > + if (__setsid () == -1) > + return -1; > > - if ((fd = __open_nocancel(_PATH_DEVNULL, O_RDWR, 0)) != -1 > - && __glibc_likely (__fstat64_time64 (fd, &st) == 0)) { > - if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0 > + if (!nochdir) > + (void) __chdir ("/"); I think there is no need to ignore return code. > + > + if (!noclose) > + { > + struct __stat64_t64 st; > + > + fd = __open_nocancel(_PATH_DEVNULL, O_RDWR, 0); Missing space before '('. > + if (fd != -1 && __glibc_likely (__fstat64_time64 (fd, &st) == 0)) > + { > + if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0 > #if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR > - && (st.st_rdev > - == makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR)) > + && (st.st_rdev == makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR)) > #endif > - ) { > - (void)__dup2(fd, STDIN_FILENO); > - (void)__dup2(fd, STDOUT_FILENO); > - (void)__dup2(fd, STDERR_FILENO); > - if (fd > 2) > - (void)__close (fd); > - } else { > - /* We must set an errno value since no > - function call actually failed. */ > - __close_nocancel_nostatus (fd); > - __set_errno (ENODEV); > - return -1; > - } > - } else { > - __close_nocancel_nostatus (fd); > - return -1; > - } > - } > - return (0); > + ) > + { > + (void) __dup2 (fd, STDIN_FILENO); > + (void) __dup2 (fd, STDOUT_FILENO); > + (void) __dup2 (fd, STDERR_FILENO); > + if (fd > 2) > + (void) __close (fd); > + } > + else > + { > + /* We must set an errno value since no function call > + actually failed. */ > + __close_nocancel_nostatus (fd); > + __set_errno (ENODEV); > + return -1; > + } > + } > + else > + { > + __close_nocancel_nostatus (fd); > + return -1; > + } > + } > + > + return 0; > }
On Tue, Apr 18, 2023 at 8:02 AM Adhemerval Zanella Netto via Libc-alpha < libc-alpha@sourceware.org> wrote: > > > On 17/04/23 19:58, Sergey Bugaev via Libc-alpha wrote: > > This is nicer, and is going to be required for the following changes > > to reasonably stay within the 79 column limit. > > > > No functional change. > > > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> > > LGTM, some minor nits below. > > > --- > > misc/daemon.c | 88 ++++++++++++++++++++++++++++----------------------- > > 1 file changed, 49 insertions(+), 39 deletions(-) > > > > diff --git a/misc/daemon.c b/misc/daemon.c > > index 3c73ac2a..61da49b7 100644 > > --- a/misc/daemon.c > > +++ b/misc/daemon.c > > @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c 8.1 > (Berkeley) 6/4/93"; > > > I think there is no need to ignore return code. > > > Also This code clearly comes from freeBSD.. which has since updated the code to ignore SIGHUP when the parent exits. https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c
On 18/04/23 10:48, Cristian Rodríguez wrote: > > > On Tue, Apr 18, 2023 at 8:02 AM Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org <mailto:libc-alpha@sourceware.org>> wrote: > > > > On 17/04/23 19:58, Sergey Bugaev via Libc-alpha wrote: > > This is nicer, and is going to be required for the following changes > > to reasonably stay within the 79 column limit. > > > > No functional change. > > > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com <mailto:bugaevc@gmail.com>> > > LGTM, some minor nits below. > > > --- > > misc/daemon.c | 88 ++++++++++++++++++++++++++++----------------------- > > 1 file changed, 49 insertions(+), 39 deletions(-) > > > > diff --git a/misc/daemon.c b/misc/daemon.c > > index 3c73ac2a..61da49b7 100644 > > --- a/misc/daemon.c > > +++ b/misc/daemon.c > > @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c 8.1 (Berkeley) 6/4/93"; > > > I think there is no need to ignore return code. > > > Also This code clearly comes from freeBSD.. which has since updated the code to ignore SIGHUP when the parent exits. > > https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c <https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c> I think such change should be in a different patch though.
On Tue, Apr 18, 2023 at 9:49 PM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > > > > On 18/04/23 10:48, Cristian Rodríguez wrote: > > > > > > On Tue, Apr 18, 2023 at 8:02 AM Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org <mailto:libc-alpha@sourceware.org>> wrote: > > > > > > > > On 17/04/23 19:58, Sergey Bugaev via Libc-alpha wrote: > > > This is nicer, and is going to be required for the following changes > > > to reasonably stay within the 79 column limit. > > > > > > No functional change. > > > > > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com <mailto:bugaevc@gmail.com>> > > > > LGTM, some minor nits below. > > > > > --- > > > misc/daemon.c | 88 ++++++++++++++++++++++++++++----------------------- > > > 1 file changed, 49 insertions(+), 39 deletions(-) > > > > > > diff --git a/misc/daemon.c b/misc/daemon.c > > > index 3c73ac2a..61da49b7 100644 > > > --- a/misc/daemon.c > > > +++ b/misc/daemon.c > > > @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c 8.1 (Berkeley) 6/4/93"; > > > > > > I think there is no need to ignore return code. > > > > > > Also This code clearly comes from freeBSD.. which has since updated the code to ignore SIGHUP when the parent exits. > > > > https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c <https://web.mit.edu/freebsd/head/lib/libc/gen/daemon.c> > > I think such change should be in a different patch though. I'm going to send out a v2 of this series with the proposed changes to daemon () included (in a separate patch from reformatting it). It is my understanding, though, that setting and resetting the signal handler like that is racy, since signal delivery is asynchronous. We may get a SIGHUP some time after daemon () completes, even. Sergey
diff --git a/misc/daemon.c b/misc/daemon.c index 3c73ac2a..61da49b7 100644 --- a/misc/daemon.c +++ b/misc/daemon.c @@ -43,50 +43,60 @@ static char sccsid[] = "@(#)daemon.c 8.1 (Berkeley) 6/4/93"; int daemon (int nochdir, int noclose) { - int fd; + int fd; - switch (__fork()) { - case -1: - return (-1); - case 0: - break; - default: - _exit(0); - } + switch (__fork ()) + { + case -1: + return -1; - if (__setsid() == -1) - return (-1); + case 0: + break; - if (!nochdir) - (void)__chdir("/"); + default: + _exit (0); + } - if (!noclose) { - struct __stat64_t64 st; + if (__setsid () == -1) + return -1; - if ((fd = __open_nocancel(_PATH_DEVNULL, O_RDWR, 0)) != -1 - && __glibc_likely (__fstat64_time64 (fd, &st) == 0)) { - if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0 + if (!nochdir) + (void) __chdir ("/"); + + if (!noclose) + { + struct __stat64_t64 st; + + fd = __open_nocancel(_PATH_DEVNULL, O_RDWR, 0); + if (fd != -1 && __glibc_likely (__fstat64_time64 (fd, &st) == 0)) + { + if (__builtin_expect (S_ISCHR (st.st_mode), 1) != 0 #if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR - && (st.st_rdev - == makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR)) + && (st.st_rdev == makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR)) #endif - ) { - (void)__dup2(fd, STDIN_FILENO); - (void)__dup2(fd, STDOUT_FILENO); - (void)__dup2(fd, STDERR_FILENO); - if (fd > 2) - (void)__close (fd); - } else { - /* We must set an errno value since no - function call actually failed. */ - __close_nocancel_nostatus (fd); - __set_errno (ENODEV); - return -1; - } - } else { - __close_nocancel_nostatus (fd); - return -1; - } - } - return (0); + ) + { + (void) __dup2 (fd, STDIN_FILENO); + (void) __dup2 (fd, STDOUT_FILENO); + (void) __dup2 (fd, STDERR_FILENO); + if (fd > 2) + (void) __close (fd); + } + else + { + /* We must set an errno value since no function call + actually failed. */ + __close_nocancel_nostatus (fd); + __set_errno (ENODEV); + return -1; + } + } + else + { + __close_nocancel_nostatus (fd); + return -1; + } + } + + return 0; }
This is nicer, and is going to be required for the following changes to reasonably stay within the 79 column limit. No functional change. Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> --- misc/daemon.c | 88 ++++++++++++++++++++++++++++----------------------- 1 file changed, 49 insertions(+), 39 deletions(-)