[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/4] os-posix: Add os_set_daemonize()
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v2 1/4] os-posix: Add os_set_daemonize() |
Date: |
Fri, 4 Mar 2022 11:58:03 +0000 |
User-agent: |
Mutt/2.1.5 (2021-12-30) |
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 :|
- [PATCH v2 0/4] qsd: Add --daemonize; and add job quit tests, Hanna Reitz, 2022/03/03
- [PATCH v2 2/4] qsd: Add pre-init argument parsing pass, Hanna Reitz, 2022/03/03
- [PATCH v2 3/4] qsd: Add --daemonize, Hanna Reitz, 2022/03/03
- [PATCH v2 4/4] iotests/185: Add post-READY quit tests, Hanna Reitz, 2022/03/03
- Re: [PATCH v2 0/4] qsd: Add --daemonize; and add job quit tests, Kevin Wolf, 2022/03/04