Subject: Re: bug#60788: [PATCH] services: Add vnstat-service-type.

From: Ludovic Courtès <[email-masked]>
Date: Fri, 07 Apr 2023 17:22:25 +0200

Hi!

Bruno Victal <[email-masked]> skribis:

> * gnu/services/monitoring.scm (vnstat-service-type): New variable.
> * doc/guix.texi (Monitoring Services): Document it.
> ---
>
> Changes since v8:
> * Forgot to amend commit in v8, v9 corrects this.

Sorry to chime in after 9 versions (!).

I think a system test would be nice, we generally require it upfront,
but since Maxim wrote it can come later, let’s not let it block this
patch any longer.

One comment:

> +@item @code{database-dir} (default: @code{"/var/lib/vnstat"}) (type: string)

[...]

> +@item @code{create-dirs?} (default: @code{#t}) (type: maybe-boolean)

For consistency, both within this record and with the rest of Guix, I
suggest avoiding abbreviations.  Since this will be part of the API,
better fix it now than later.

> +@item @code{max-bandwidth} (type: maybe-integer)
> +Maximum bandwidth for all interfaces.  If the interface specific traffic
> +exceeds the given value then the data is assumed to be invalid and
> +rejected.  Set to 0 in order to disable the feature.  Value range:
> +@samp{0}..@samp{50000}
> +
> +@item @code{max-bw} (type: maybe-alist)
> +Same as @var{max-bandwidth} but can be used for setting individual
> +limits for selected interfaces.  This is an association list of
> +interfaces as symbols/strings to integer values.  For example,
> +@lisp
> +(max-bw `((eth0 .  15000)
> +          (ppp0 .  10000)))
> +@end lisp

Both the naming and semantics are a bit confusing to me.

How about s/max-bw/per-interface-max-bandwidth/ ?

Side note: I’d represent interfaces as strings because there’s no
guarantee they “fit” in a symbol.

That’s all, thanks!

Ludo’.