[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: -o option for start-stop-daemon
From:       Louai Al-Khanji <louai () astranis ! com>
Date:       2023-11-08 17:37:06
Message-ID: CACvKkQv8GeLvKywMsDz5St_hac02-crqwO5WoqB16pfE=F1twg () mail ! gmail ! com
[Download RAW message or body]

On Wed, Nov 8, 2023 at 5:08 AM Denys Vlasenko <vda.linux@googlemail.com> wrote:
>
> On Tue, Nov 7, 2023 at 7:27 PM Louai Al-Khanji <louai@astranis.com> wrote:
> > On Tue, Nov 7, 2023 at 8:03 AM Denys Vlasenko <vda.linux@googlemail.com> wrote:
> > > On Fri, Nov 3, 2023 at 11:31 PM Louai Al-Khanji <louai@astranis.com> wrote:
> > > > Attached is a proposed patch. Any feedback would be appreciated.
> > >
> > > My experiments with ssd version 1.21.22 show that the file is opened with
> > > O_CREAT|O_APPEND, and it does not allow -O without -b.
> > >
> > > If execv fails, error message goes to this file.
> > > IOW: there is no need to save/restore old stderr fd. Just replace it
> > > with the new fd
> > > (and don't forget to not leak any extra open fds).
> >
> > Thank you for the feedback everyone. New version attached.
> >
> > It looked a little tricky to me to add the logic around
> > bb_daemon_helper() since it closes open fds. Maybe I am missing
> > something.
> >
> > The code now checks the args more strictly and prints usage if -O is
> > given without -b.
> >
> > I dropped restoring of the stdout/stderr fds. I believe this patch
> > cannot leak fds.
> >
> > One question I have is whether it's okay to lose error messages. On
> > failure to open the output file I believe the error message currently
> > goes into the void. Same if any of the dup2 calls or the close call
> > fails.
>
> Yes, that's not good.
> I tried to fix it in git now. Please try.
>

The new behavior seems correct to me - if the output file cannot be
opened an error is printed at invocation site. If the exec fails it
goes to the requested output file. Thank you for adding the -O
functionality, it is very handy for my use case.

> > BTW I noticed that bb_daemon_helper() internally calls setsid()
> > already, so the extra call in start_stop_daemon.c seems superfluous. I
> > didn't however touch that in this patch.
>
> No:
>
> #define bb_daemon_helper(arg) bb_daemonize_or_rexec((arg) |
> DAEMON_ONLY_SANITIZE, NULL)
>
> DAEMON_ONLY_SANITIZE bit prevents setsid() and daemonization (the vfork).

Ah, I missed that the macro set the flag. I see you added a comment to
catch future readers, thank you.

-- 


________
This email and any attachments may contain Astranis confidential 
and/or proprietary information governed by a non-disclosure agreement, and 
are intended solely for the individual or entity specified by the message.
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[prev in list] [next in list] [prev in thread] [next in thread]