From: Bruno Victal <mirai@makinata.eu>
To: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Cc: Maxim Cournoyer <maxim.cournoyer@gmail.com>, 60788@debbugs.gnu.org
Subject: [bug#60788] [PATCH] services: Add vnstat-service-type.
Date: Mon, 16 Jan 2023 19:31:10 +0000 [thread overview]
Message-ID: <7d22fea8-112d-1c01-6b6a-6cfe4e0fa9e3@makinata.eu> (raw)
In-Reply-To: <87o7qy48zm.fsf@gmail.com>
On 2023-01-16 18:42, Maxim Cournoyer wrote:
>> Bruno Victal <mirai@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
next prev parent reply other threads:[~2023-01-16 19:32 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-13 20:07 [bug#60788] [PATCH] services: Add vnstat-service-type Bruno Victal
2023-01-14 21:06 ` [bug#60788] [PATCH v2] " Bruno Victal
2023-01-16 18:42 ` [bug#60788] [PATCH] " Maxim Cournoyer
2023-01-16 19:31 ` Bruno Victal [this message]
2023-01-16 19:56 ` Maxim Cournoyer
2023-01-18 0:34 ` [bug#60788] [PATCH v2] " Bruno Victal
2023-01-18 0:37 ` [bug#60788] [PATCH v2] services: vnstat: Use least-authority-wrapper Bruno Victal
2023-02-02 14:21 ` [bug#60788] [PATCH v3] services: Add vnstat-service-type Bruno Victal
2023-02-07 14:25 ` [bug#60788] [PATCH v4] " Bruno Victal
2023-02-09 3:34 ` [bug#60788] [PATCH] " Maxim Cournoyer
2023-02-09 4:19 ` Bruno Victal
2023-02-09 13:31 ` Maxim Cournoyer
2023-02-10 13:15 ` [bug#60788] [PATCH v5] " Bruno Victal
2023-02-10 14:07 ` Maxim Cournoyer
2023-02-10 14:14 ` [bug#60788] [PATCH v6] " Bruno Victal
2023-03-22 16:15 ` [bug#60788] [PATCH v7] " Bruno Victal
2023-04-03 14:14 ` [bug#60788] [PATCH v8] " Bruno Victal
2023-04-04 13:08 ` [bug#60788] [PATCH v9] " Bruno Victal
2023-04-07 15:22 ` [bug#60788] [PATCH] " Ludovic Courtès
2023-04-07 20:04 ` Maxim Cournoyer
2023-04-20 10:03 ` [bug#60788] Policy for system tests? Ludovic Courtès
2023-04-08 12:40 ` [bug#60788] [PATCH] services: Add vnstat-service-type Bruno Victal
2023-04-20 10:09 ` Ludovic Courtès
2023-05-05 0:18 ` [bug#60788] [PATCH v10 1/3] " Bruno Victal
2023-05-11 14:33 ` bug#60788: " Ludovic Courtès
2023-05-05 0:18 ` [bug#60788] [PATCH v10 2/3] services: inetd: Export accessors Bruno Victal
2023-05-05 0:18 ` [bug#60788] [PATCH v10 3/3] tests: Add vnstat tests Bruno Victal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7d22fea8-112d-1c01-6b6a-6cfe4e0fa9e3@makinata.eu \
--to=mirai@makinata.eu \
--cc=60788@debbugs.gnu.org \
--cc=maxim.cournoyer@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).