| Message ID | 20230102194156.1560195-1-selva.nair@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi, > - else if (n > 0) > + else > { > - sleep(n); > +#ifdef _WIN32 > + win32_sleep(n); > +#else > + if (n > 0) > + { > + sleep(n); My understanding is that we want to have interruptible sleep. In this case, what is the point of calling win32_sleep(0) ? I understand that management_sleep(0) is required to service possible management commands even if we don't need a delay, but shouldn't it be a no-op without management? > +void > +win32_sleep(const int n) > +{ > + if (n < 0) > + { > + return; > + } Why not <= 0 ? > + /* Sleep() is not interruptible. Use a WAIT_OBJECT to catch signal */ > + if (HANDLE_DEFINED(win32_signal.in.read)) > + { I would reverse this condition, and do just Sleep(n*1000); return; This way we get rid of the indentation level. > + time_t expire = 0; > + update_time(); > + expire = now + n; I would declare and initialize time_t expire after update_time(). > + DWORD status = WaitForSingleObject(win32_signal.in.read, (expire - now)*1000); > + if (win32_signal_get(&win32_signal) || status == WAIT_TIMEOUT) > + { > + return; > + } Shouldn't we call win32_signal_get() only if status == WAIT_OBJECT_0 ? Under what circumstances are we supposed to do the waiting again? If we get a signal, we bail out. If the wait times out, we bail out. If wait fails, we do Sleep (although at this point we probably have a bigger issue). -Lev
Hi, Thanks for the careful review On Thu, Jan 5, 2023 at 11:20 AM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > > - else if (n > 0) > > + else > > { > > - sleep(n); > > +#ifdef _WIN32 > > + win32_sleep(n); > > +#else > > + if (n > 0) > > + { > > + sleep(n); > > > My understanding is that we want to have interruptible sleep. In this case, > what is the point of calling win32_sleep(0) ? I understand that > management_sleep(0) > is required to service possible management commands even if we don't > need a delay, > but shouldn't it be a no-op without management? > Right, I'm going beyond the original which does nothing for management_sleep(0) if management is not active or enabled. I want to err on the side of checking windows signal rather than missing it. In case of unix/linux OS signals do get delivered out of band into the global signal_received. In the case of Windows, it has to be picked up (except for the obscure ctrl-Break). Note that management_sleep(0) when management is enabled does call get_signal()==win32_signal_get and picks up Windows signals, not just management commands. win32_sleep(0) also calls win32_signal_get() even if there is no time to pause. > > > +void > > +win32_sleep(const int n) > > +{ > > + if (n < 0) > > + { > > + return; > > + } > > Why not <= 0 ? > See above. > > > + /* Sleep() is not interruptible. Use a WAIT_OBJECT to catch signal > */ > > + if (HANDLE_DEFINED(win32_signal.in.read)) > > + { > > I would reverse this condition, and do just > > Sleep(n*1000); > return; > This way we get rid of the indentation level. > > > + time_t expire = 0; > > + update_time(); > > + expire = now + n; > > I would declare and initialize time_t expire after update_time(). > > + DWORD status = WaitForSingleObject(win32_signal.in.read, > (expire - now)*1000); > > + if (win32_signal_get(&win32_signal) || status == > WAIT_TIMEOUT) > > + { > > + return; > > + } > > Shouldn't we call win32_signal_get() only if status == WAIT_OBJECT_0 ? > To ensure the signal is checked at least once in all cases including n = 0. Not sure whether status will be WAIT_TIMEOUT or WAIT_OBJECT_0 in case n = 0 and there is a signal to pickup. > > Under what circumstances are we supposed to do the waiting again? If > we get a signal, we bail out. > If the wait times out, we bail out. If wait fails, we do Sleep > (although at this point we probably have a bigger issue). > Probably never -- there is a WAIT_ABANDONED which may not apply here. But, sometimes windows functions do exit with error codes not listed in the docs. As the loop is slowed down by a Sleep(1000), this can't hurt, can it? The common thread in all of the above is that I'm trying to err on the side of checking for signals an extra round as opposed to missing it. Can do a v3 if you think it's warranted. Thanks, Selva
Hi > >> >> Under what circumstances are we supposed to do the waiting again? If >> we get a signal, we bail out. >> If the wait times out, we bail out. If wait fails, we do Sleep >> (although at this point we probably have a bigger issue). >> > > Probably never -- there is a WAIT_ABANDONED which may not apply here. But, > sometimes windows functions do exit with error codes not listed in the > docs. As the loop is slowed down by a Sleep(1000), this can't hurt, can it? > I see your point now :) I had a different logic earlier where it always slept for a second before trying to wait again, but not so any longer. v3 is coming! Selva >
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 5465b7e9..9bd5ef4e 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -4045,9 +4045,16 @@ management_sleep(const int n) { management_event_loop_n_seconds(management, n); } - else if (n > 0) + else { - sleep(n); +#ifdef _WIN32 + win32_sleep(n); +#else + if (n > 0) + { + sleep(n); + } +#endif } } @@ -4088,13 +4095,18 @@ man_persist_client_stats(struct management *man, struct context *c) #else /* ifdef ENABLE_MANAGEMENT */ +#include "win32.h" void management_sleep(const int n) { +#ifdef _WIN32 + win32_sleep(n); +#else if (n > 0) { sleep(n); } +#endif /* ifdef _WIN32 */ } #endif /* ENABLE_MANAGEMENT */ diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c index c3520bca..e16d5461 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -642,50 +642,44 @@ int win32_signal_get(struct win32_signal *ws) { int ret = 0; - if (siginfo_static.signal_received) - { - ret = siginfo_static.signal_received; - } - else + + if (ws->mode == WSO_MODE_SERVICE) { - if (ws->mode == WSO_MODE_SERVICE) + if (win32_service_interrupt(ws)) { - if (win32_service_interrupt(ws)) - { - ret = SIGTERM; - } + ret = SIGTERM; } - else if (ws->mode == WSO_MODE_CONSOLE) + } + else if (ws->mode == WSO_MODE_CONSOLE) + { + switch (win32_keyboard_get(ws)) { - switch (win32_keyboard_get(ws)) - { - case 0x3B: /* F1 -> USR1 */ - ret = SIGUSR1; - break; + case 0x3B: /* F1 -> USR1 */ + ret = SIGUSR1; + break; - case 0x3C: /* F2 -> USR2 */ - ret = SIGUSR2; - break; + case 0x3C: /* F2 -> USR2 */ + ret = SIGUSR2; + break; - case 0x3D: /* F3 -> HUP */ - ret = SIGHUP; - break; + case 0x3D: /* F3 -> HUP */ + ret = SIGHUP; + break; - case 0x3E: /* F4 -> TERM */ - ret = SIGTERM; - break; + case 0x3E: /* F4 -> TERM */ + ret = SIGTERM; + break; - case 0x03: /* CTRL-C -> TERM */ - ret = SIGTERM; - break; - } - } - if (ret) - { - throw_signal(ret); /* this will update signinfo_static.signal received */ + case 0x03: /* CTRL-C -> TERM */ + ret = SIGTERM; + break; } } - return ret; + if (ret) + { + throw_signal(ret); /* this will update signinfo_static.signal received */ + } + return (siginfo_static.signal_received); } void @@ -1603,4 +1597,40 @@ set_openssl_env_vars() } } +void +win32_sleep(const int n) +{ + if (n < 0) + { + return; + } + + /* Sleep() is not interruptible. Use a WAIT_OBJECT to catch signal */ + if (HANDLE_DEFINED(win32_signal.in.read)) + { + time_t expire = 0; + update_time(); + expire = now + n; + while (expire >= now) + { + DWORD status = WaitForSingleObject(win32_signal.in.read, (expire - now)*1000); + if (win32_signal_get(&win32_signal) || status == WAIT_TIMEOUT) + { + return; + } + + update_time(); + + if (status == WAIT_FAILED && expire > now) + { + Sleep((expire-now)*1000); + return; + } + } + } + else + { + Sleep(n*1000); + } +} #endif /* ifdef _WIN32 */ diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h index b1371999..26d5ef8f 100644 --- a/src/openvpn/win32.h +++ b/src/openvpn/win32.h @@ -330,5 +330,8 @@ openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned in bool openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...); +/* Sleep that can be interrupted by signals and exit event */ +void win32_sleep(const int n); + #endif /* ifndef OPENVPN_WIN32_H */ #endif /* ifdef _WIN32 */