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
