Re: [PATCH v2 1/4] os-posix: Add os_set_daemonize()

[Kevin Wolf]

Am 04.03.2022 um 10:19 hat Daniel P. Berrangé geschrieben:
> On Thu, Mar 03, 2022 at 05:48:11PM +0100, Hanna Reitz wrote:
> > The daemonizing functions in os-posix (os_daemonize() and
> > os_setup_post()) only daemonize the process if the static `daemonize`
> > variable is set.  Right now, it can only be set by os_parse_cmd_args().
> > 
> > In order to use os_daemonize() and os_setup_post() from the storage
> > daemon to have it be daemonized, we need some other way to set this
> > `daemonize` variable, because I would rather not tap into the system
> > emulator's arg-parsing code.  Therefore, this patch adds an
> > os_set_daemonize() function, which will return an error on os-win32
> > (because daemonizing is not supported there).
> 
> IMHO the real flaw here is the design of 'os_daemonize' in that it
> relies on static state. If I see a call to a function 'os_daemonize()'
> I expect to be daemonized on return, but with this design that is not
> guaranteed which is a big surprise.
> 
> I'd suggest we push the condition into the caller instead of adding
> this extra function, so we have the more sane pattern:
> 
>    if (daemonmize()) {
>       os_daemonize()
>    }

It's not as simple, the static daemonize variable is used in more places
than just os_daemonize(). I'm not sure if it's worth changing how all of
this works, but if we did, it would be a refactoring mostly focussed on
the system emulator and an issue separate from adding the option to the
storage daemon.

Kevin