| Message ID | 20220303164814.284974-2-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qsd: Add --daemonize; and add job quit tests | expand |
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). > > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > +++ b/include/sysemu/os-win32.h > @@ -77,6 +77,11 @@ typedef struct { > } qemu_timeval; > int qemu_gettimeofday(qemu_timeval *tp); > > +static inline int os_set_daemonize(bool d) > +{ > + return -ENOTSUP; Should this fail only if d is true? Or will all callers only ever pass true, in which case why do we need the paraemeter? > +} > + > static inline bool is_daemonized(void) > { > return false; > diff --git a/os-posix.c b/os-posix.c > index ae6c9f2a5e..24692c8593 100644 > --- a/os-posix.c > +++ b/os-posix.c > @@ -317,6 +317,12 @@ bool is_daemonized(void) > return daemonize; > } > > +int os_set_daemonize(bool d) > +{ > + daemonize = d; > + return 0; > +} > + > int os_mlock(void) > { > #ifdef HAVE_MLOCKALL > -- > 2.34.1 > >
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
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
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
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h index 2edf33658a..dd64fb401d 100644 --- a/include/sysemu/os-posix.h +++ b/include/sysemu/os-posix.h @@ -55,6 +55,7 @@ int os_mlock(void); typedef struct timeval qemu_timeval; #define qemu_gettimeofday(tp) gettimeofday(tp, NULL) +int os_set_daemonize(bool d); bool is_daemonized(void); /** diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h index 43f569b5c2..68af96907e 100644 --- a/include/sysemu/os-win32.h +++ b/include/sysemu/os-win32.h @@ -77,6 +77,11 @@ typedef struct { } qemu_timeval; int qemu_gettimeofday(qemu_timeval *tp); +static inline int os_set_daemonize(bool d) +{ + return -ENOTSUP; +} + static inline bool is_daemonized(void) { return false; diff --git a/os-posix.c b/os-posix.c index ae6c9f2a5e..24692c8593 100644 --- a/os-posix.c +++ b/os-posix.c @@ -317,6 +317,12 @@ bool is_daemonized(void) return daemonize; } +int os_set_daemonize(bool d) +{ + daemonize = d; + return 0; +} + int os_mlock(void) { #ifdef HAVE_MLOCKALL
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). Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- include/sysemu/os-posix.h | 1 + include/sysemu/os-win32.h | 5 +++++ os-posix.c | 6 ++++++ 3 files changed, 12 insertions(+)