Hi, Thanks for the careful review
On Thu, Jan 5, 2023 at 11:20 AM Lev Stipakov <lstipa...@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
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel