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

[Daniel P . Berrangé]

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

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|