On Fri, Mar 04, 2022 at 11:20:39AM +0100, Kevin Wolf wrote: > 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.
It isn't that difficult to do the refactoring needed, so I've just sent a series that does the job and CC folks from this thread on it. With 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 :|