GNU bug report logs - #60788
[PATCH] services: Add vnstat-service-type.

Previous Next

Full log

Message #14 received at 60788 <at> debbugs.gnu.org (full text, mbox):

From: Bruno Victal <mirai <at> makinata.eu>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, 60788 <at> debbugs.gnu.org
Subject: Re: bug#60788: [PATCH] services: Add vnstat-service-type.
Date: Mon, 16 Jan 2023 19:31:10 +0000

On 2023-01-16 18:42, Maxim Cournoyer wrote:

>> Bruno Victal <mirai <at> makinata.eu> writes:
>> +;;;
>> +;;; vnstat daemon
>> +;;;
>> +
>> +(define* (camelfy-field-name field-name #:key (dromedary? #f))
>> +  (match (string-split (symbol->string field-name) #\-)
>> +    ((head tail ...)
>> +     (string-join (cons (if dromedary? head (string-upcase head 0 1))
>> +                        (map string-capitalize tail)) ""))))
> 
> I'd name this pascal-case-field-name, and drop the apparently unused
> dromerady? argument (fun, though :-)).  If we need it, we could
> introduce a 2nd camel-case-field-name.

I was anticipating that this snippet would get reused (i.e. copy-pasted) by
other services so I wanted to generalize it before it starts spawning
its dromedary cousin.

If this isn't preferred, I can drop it and just leave it as
pascal-case-field-name. (maybe these kinds of procedures could go into
a new "configuration utils" module with useful auxiliary functions?)

> 
>> +(define (dock-field-name field-name)
>> +  "Drop rightmost '?' character"
>> +  (let ((str (symbol->string field-name)))
>> +    (if (string-suffix? "?" str)
>> +        (string->symbol (string-drop-right str 1))
>> +        field-name)))
> 
> I'm not sure about the meaning of 'dock' in this procedure name.
> Perhaps 'strip-trailing-?-character' would be better?

I couldn't think of any other word that would essentially describe
whats amounts to "dropping" the tail of a list/string.
(here, 'dock': "the practice of cutting off or trimming the tail of an animal")

>> +;; Documentation strings from vnstat.conf manpage adapted to texinfo.
>> +;; vnstat checkout: v2.10, commit b3408af1c609aa6265d296cab7bfe59a61d7cf70
>> +(define-configuration vnstat-configuration
>> +  (package
>> +    (file-like vnstat)
>> +    "The vnstat package."
>> +    empty-serializer)
>> +
>> +  (database-dir
>> +   (string "/var/lib/vnstat")
>> +   "\
>> +Specifies the directory where the database is to be stored.
>> +A full path must be given and a leading '/' isn't required.")  
>> +
>> +  (5-minute-hours
>> +   (maybe-integer 48)
>> +   "\
>> +Data retention duration for the 5 minute resolution entries. The configuration
>> +defines for how many past hours entries will be stored. Set to @code{-1} for
>> +unlimited entries or to @code{0} to disable the data collection of this
>> +resolution.")
>> +
>> +  (64bit-interface-counters
>> +   (maybe-integer -2)
>> +   "\
>> +Select interface counter handling. Set to @code{1} for defining that all interfaces
>> +use 64-bit counters on the kernel side and @code{0} for defining 32-bit counter. Set
>> +to @code{-1} for using the old style logic used in earlier versions where counter
>> +values within 32-bits are assumed to be 32-bit and anything larger is assumed to
>> +be a 64-bit counter. This may produce false results if a 64-bit counter is
>> +reset within the 32-bits. Set to @code{-2} for using automatic detection based on
>> +available kernel datastructures.")
> 
> Please reflow these paragraphs so they fit under the 80 characters width
> limit.  Perhaps drop the \ escape, as it doesn't provide much and looks
> a bit worst, in my opinion.  Each sentence should be separated by two
> spaces (that's a Texinfo convention).  This comment applies to all
> similar documentation texts.

They're formatted this way as I find it easier to compare against upstream troff sources.
If they're reflowed or if the \ escape is dropped then the diffs get larger to compare against
(most of the changes here are easier to compare using word-diffs)

>> +  (always-add-new-interfaces?
>> +   (maybe-boolean #t)
>> +   "\
>> +Enable or disable automatic creation of new database entries for interfaces not
>> +currently in the database even if the database file already exists when the
>> +daemon is started. New database entries will also get created for new interfaces
>> +seen while the daemon is running. Pseudo interfaces lo, lo0 and sit0 are always
>> +excluded from getting added.")
> 
> The lo, lo0 and sit0 could use a @samp{} or decorator.

Noted.

> 
> [...]
> 
>> +(define (vnstat-serialize-configuration config)
>> +  (mixed-text-file
>> +   "vnstat.conf"
>> +   (serialize-configuration config vnstat-configuration-fields)))
>> +
>> +(define (vnstat-shepherd-service config)
>> +  (let ((config-file (vnstat-serialize-configuration config)))
>> +    (match-record config <vnstat-configuration> (package pid-file)
>> +      (shepherd-service
>> +       (documentation "Run vnstatd.")
>> +       (requirement `(networking))
>> +       (provision '(vnstatd))
>> +       (start #~(make-forkexec-constructor
>> +                 (list #$(file-append package "/sbin/vnstatd")
>> +                       "--daemon"
>> +                       "--config" #$config-file)
>> +                 #:pid-file #$pid-file))
>> +       (stop #~(make-kill-destructor))
>> +       (actions
>> +        (list (shepherd-configuration-action config-file)
> 
> I don't understand what (shepherd-configuration-action config-file)
> corresponds to?

It shows the path to the config file with `herd configuration vnstatd'.
It was introduced with 'ebc7de6a1efb702fd0364128cbde19697966c4f4'.

>> +              (shepherd-action
>> +               (name 'reload)
>> +               (documentation "Reload vnstatd.")
>> +               (procedure
>> +                #~(lambda (pid)
>> +                    (if pid
>> +                        (begin
>> +                          (kill pid SIGHUP)
>> +                          (format #t
>> +                                  "Issued SIGHUP to vnstatd (PID ~a)."
>> +                                  pid))
>> +                        (format #t "vnstatd is not running.")))))))))))
> 
>> +(define (vnstat-account-service config)
>> +  (match-record config <vnstat-configuration> (daemon-group daemon-user)
>> +    (if (every maybe-value-set? (list daemon-group daemon-user))
>> +        (list
>> +         (user-group
>> +          (name daemon-group)
>> +          (system? #t))
>> +         (user-account
>> +          (name daemon-user)
>> +          (group daemon-group)
>> +          (system? #t)
>> +          (home-directory "/var/empty")
>> +          (shell (file-append shadow "/sbin/nologin"))))
>> +        '())))
>> +
>> +(define vnstat-service-type
>> +  (service-type
>> +   (name 'vnstat)
>> +   (description "vnStat network-traffic monitor service.")
>> +   (extensions
>> +    (list (service-extension shepherd-root-service-type
>> +                             (compose list vnstat-shepherd-service))
>> +          (service-extension account-service-type
>> +                             vnstat-account-service)))
>> +   (default-value (vnstat-configuration))))
> 
> The rest LGTM (a system test would be nice, but since the Shepherd
> service definition is straightforward, it could come later, when the
> need arise).

I've thought a bit about the system test though the way vnstat works is that
it needs to collect some data and it also takes a while for the daemon to flag it
as ready (~5 minutes). This would be a somewhat slow and non-trivial test to write.


WDYT?



Cheers,
Bruno

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.