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
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
