GNU bug report logs - #75145
[PATCH] services: NetworkManager: configuration-directory

Previous Next

Full log

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

From: Arnaud Daby-Seesaram <ds-ac <at> nanein.fr>
To: 45mg <45mg.writes <at> gmail.com>
Cc: Ludovic Courtès <ludo <at> gnu.org>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, 75145 <at> debbugs.gnu.org
Subject: Re: [PATCH v3] services: network-manager: Add
 extra-configuration-files field.
Date: Sun, 02 Feb 2025 00:31:16 +0100

[Message part 1 (text/plain, inline)]

Hi,

Thank you for the v3.  I just have two minor comments.


45mg <45mg.writes <at> gmail.com> writes:

> @@ -1253,18 +1254,26 @@ (define-record-type* <network-manager-configuration>
>                 (default '()))
>    (iwd? network-manager-configuration-iwd?  ; TODO: deprecated field, remove.
>          (default #f)
> -        (sanitize warn-iwd?-field-deprecation)))
> +        (sanitize warn-iwd?-field-deprecation))
> +  (extra-configuration-files network-manager-configuration-extra-configuration-files
> +                             (default '())))  ;alist of file names to file-like objects
>  
>  (define (network-manager-activation config)
>    ;; Activation gexp for NetworkManager
>    (match-record config <network-manager-configuration>
> -    (network-manager dns vpn-plugins)
> +                (network-manager dns vpn-plugins extra-configuration-files)
>      #~(begin
>          (use-modules (guix build utils))
>          (mkdir-p "/etc/NetworkManager/system-connections")
>          #$@(if (equal? dns "dnsmasq")
>                 ;; create directory to store dnsmasq lease file
>                 '((mkdir-p "/var/lib/misc"))
> +               '())
> +        #$@(if extra-configuration-files
> +               `((symlink
> +                  ,(file-union "network-manager-configuration-directory"
> +                               extra-configuration-files)
> +                  "/etc/NetworkManager/conf.d"))
>                 '()))))

If I understand, the `if' is here to avoid creating a symbolic link to
an empty directory: if no extra configuration files are provided, do not
symlink.  However, in Guile, it seems that (if '() 'yes 'no) evaluates
to 'yes.  Therefore, the symlink is always created.  A solution is to
use `pair?':
--8<---------------cut here---------------start------------->8---
        #$@(if (pair? extra-configuration-files)
               `((symlink
                  ,(file-union "network-manager-configuration-directory"
                               extra-configuration-files)
                  "/etc/NetworkManager/conf.d"))
               '()))))
--8<---------------cut here---------------end--------------->8---

Apologies, I should have realised that before my first review.


> +@item @code{extra-configuration-files} (default: @code{'()})
> +An alist of file names to file-like objects, representing configuration
> +files which will be added to @file{/etc/NetworkManager/conf.d}.
> +NetworkManager will read additional configuration from this directory.
> +For details on configuration file precedence and the configuration file
> +format, see the @command{NetworkManager.conf(5)} man page.

I am not sure that "alist" is the correct term.  When I hear alist, I
think of something of the form
((key-1 . val-1)
 ...
 (key-n . val-n))

The documentation of `file-union' uses the term "two-element list" for
its argument.  Maybe it would be more precise to do the same (I am not
very opinionated on this).


Best regards,

-- 
Arnaud

[signature.asc (application/pgp-signature, inline)]

Previous Next

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