[Openvpn-devel] Document/cleanup event_timeout functions

  Remove function event_timeout_clear_ret as it is unused.

Cleanup event_timeout_trigger a bit. Do an instant return false if the
timeout is not defined and inline local_now and use
event_timeout_remaining instead of local duplicated code.

Add doxygen comments for all timeout function, especially for the
event_timeout_trigger function that is hard to understand otherwise.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/interval.c | 50 ++++++++++++-----------
 src/openvpn/interval.h | 92 +++++++++++++++++++++++++++++-------------
 src/openvpn/openvpn.h  |  4 +-
 3 files changed, 92 insertions(+), 54 deletions(-)
  

On Fri, Sep 23, 2022 at 10:58:04AM +0200, Arne Schwabe wrote:
> Remove function event_timeout_clear_ret as it is unused.
> 
> Cleanup event_timeout_trigger a bit. Do an instant return false if the
> timeout is not defined and inline local_now and use
> event_timeout_remaining instead of local duplicated code.

That part best seen with git diff -w ;)
Changes look good to me.

> Add doxygen comments for all timeout function, especially for the
> event_timeout_trigger function that is hard to understand otherwise.

Some comments on those below.

> diff --git a/src/openvpn/interval.h b/src/openvpn/interval.h
> index f58bfacf6..f6e40705c 100644
> --- a/src/openvpn/interval.h
> +++ b/src/openvpn/interval.h
> @@ -135,9 +135,9 @@ interval_action(struct interval *top)
>  
>  struct event_timeout
>  {
> -    bool defined;
> -    interval_t n;
> -    time_t last; /* time of last event */
> +    bool defined;       /**< This timeout is active */
> +    interval_t n;       /**< periodic interval for periodic timeouts */
> +    time_t last;        /**< time of last event */
>  };
>  
>  static inline bool
> @@ -145,7 +145,12 @@ event_timeout_defined(const struct event_timeout *et)
>  {
>      return et->defined;
>  }
> -
> +/**
> + * Clears the timeout and reset all values to 0. Following timer checks will
> + * not trigger.
> + *
> + * @param et    timer struct
> + */
>  static inline void
>  event_timeout_clear(struct event_timeout *et)
>  {
> @@ -154,22 +159,30 @@ event_timeout_clear(struct event_timeout *et)
>      et->last = 0;
>  }
>  
> -static inline struct event_timeout
> -event_timeout_clear_ret(void)
> -{
> -    struct event_timeout ret;
> -    event_timeout_clear(&ret);
> -    return ret;
> -}
>  
> +/**
> + * Initialises a timer struct. The timer will become true/trigger after last + n seconds

Add period for consistency.

> + *
> + *
> + * @param et            Timer struct
> + * @param n             Interval of the timer for periodic timer. A negative value for n will be interpreted as 0

Add period for consistency. Wrap text? 113 is a bit broad.

> + * @param last          Sets the base time of the timer.
> + */
>  static inline void
> -event_timeout_init(struct event_timeout *et, interval_t n, const time_t local_now)
> +event_timeout_init(struct event_timeout *et, interval_t n, const time_t last)
>  {
>      et->defined = true;
>      et->n = (n >= 0) ? n : 0;
> -    et->last = local_now;
> +    et->last = last;
>  }
>  
> +/**
> + * Reset a timer.

"Resets" for consistency.

> + *
> + * Sets the last time the timer has been triggered for the calculation of the
> + * next event.
> + * @param et
> + */
>  static inline void
>  event_timeout_reset(struct event_timeout *et)
>  {
> @@ -179,6 +192,12 @@ event_timeout_reset(struct event_timeout *et)
>      }
>  }
>  
> +/**
> + * Sets the periodic parameter n of a timeout.

"interval" instead of "periodic parameter" for consistency?

> + * @param et
> + * @param n set the periodic value of a timeout, negative values
> + *          will be interpreted as 0

Redundant description. Maybe more like

* @param n  Interval of the timer. A negative value for n will be interpreted as 0

> + */
>  static inline void
>  event_timeout_modify_wakeup(struct event_timeout *et, interval_t n)
>  {

Just below here there is an additional comment inside the function:
/* note that you might need to call reset_coarse_timers after this */

That should be moved into the doxygen comment.

> @@ -189,32 +208,47 @@ event_timeout_modify_wakeup(struct event_timeout *et, interval_t n)
>      }
>  }
>  
> -/*
> - * Will return the time left for a timeout, this function does not check
> - * if the timeout is actually valid
> +/**
> + * Return time until the timeout should triggered from from now,
> + * this function does not check if the timeout is actually valid.
>   */

"Returns" for consistency.
"now, this" -> "now. This"

>  static inline interval_t
>  event_timeout_remaining(struct event_timeout *et)
>  {
> -    return (interval_t) (et->last - now + et->n);
> +    return (interval_t) ((et->last + et->n) - now);
>  }
>  
> -/*
> - * This is the principal function for testing and triggering recurring
> - * timers and will return true on a timer signal event.
> - * If et_const_retry == ETT_DEFAULT and a signal occurs,
> - * the function will return true and *et will be armed for the
> - * next event.  If et_const_retry >= 0 and a signal occurs,
> - * *et will not be touched, but *tv will be set to
> - * minimum (*tv, et_const_retry) for a future re-test,
> - * and the function will return true.
> - */
> -
>  #define ETT_DEFAULT (-1)
>  
> +/**
> + * This is the principal function for testing and triggering recurring
> + * timers.
> + *
> + * If *et is not triggered *tv is set to remaining time until the timeout if
> + * not already lower:
> + *
> + *      *tv = minimum(*tv, event_timeout_remaining(*et))
> + *
> + * If *et triggers and et_const_retry is negative (ETT_DEFAULT is -1):
> + *  - the function will return true
> + *  - *et will be armed for the next event (et->last set to now).
> + *  - *tv will be lowered to the event period (n) if larger than the
> + *     period of the event (set to *et's next timeout)
> + *
> + * If *et triggers and et_const_retry >= 0, *tv be reduced to et_const_try if

"will be"

> + * larger:
> + *
> + *    *tv = *minimum(*tv, et_const_retry)

I think this should explicitly mention that et is NOT re-armed and explain why
you would want to use that.

> + *
> + *
> + * @param et                the timeout to check
> + * @param tv                will be set to timeout for next check for this timeout unless already small.

"smaller"

> + * @param et_const_retry    see above

"above" does not actually explain why you would use this. See above ;)

> + * @return                  if the timeout has triggered and event has been reset
> + */

You're mixing lowered/lower and reduced/larger/smaller. I think it would be better
to only use one of those.

>  bool event_timeout_trigger(struct event_timeout *et,
>                             struct timeval *tv,
> -                           const int et_const_retry);
> +                           int et_const_retry);

Why remove the const here? You did not remove it in interval.c

>  
>  /*
>   * Measure time intervals in microseconds
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index 00cd652fa..aae1d21b8 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -386,7 +386,9 @@ struct context_2
>       * Event loop info
>       */
>  
> -    /* how long to wait on link/tun read before we will need to be serviced */
> +    /* time to next event of timers and similar. This is used to determine

"Time"

> +     * how long to wait on event wait (select/poll on link/tun read)
> +     * before this context wants to be serviced */

Add period for consistency

>      struct timeval timeval;
>  
>      /* next wakeup for processing coarse timers (>1 sec resolution) */
> -- 
> 2.37.0 (Apple Git-136)

Regards,
  

Am 23.09.22 um 12:12 schrieb Frank Lichtenheld:

> 
>>   bool event_timeout_trigger(struct event_timeout *et,
>>                              struct timeval *tv,
>> -                           const int et_const_retry);
>> +                           int et_const_retry);
> 
> Why remove the const here? You did not remove it in interval.c

clang-tidy complains here. The reason is that the const is doing
nothing in the prototype:

Clang-Tidy: Parameter 'et_const_retry' is const-qualified in the 
function declaration; const-qualification of parameters only has an 
effect in function definitions.

Arne