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

Previous Next

Full log

View this message in rfc822 format

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

Hi Arnaud,

Arnaud Daby-Seesaram <ds-ac <at> nanein.fr> writes:

> 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?':
>
>         #$@(if (pair? extra-configuration-files)
>                `((symlink
>                   ,(file-union "network-manager-configuration-directory"
>                                extra-configuration-files)
>                   "/etc/NetworkManager/conf.d"))
>                '()))))
>
> Apologies, I should have realised that before my first review.

That's fine, and good catch!

>> +@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).

Indeed, I think a list of pure lists (rather than pair) cannot be called
an association list (alist), as that shoud be made up of pairs (using
cons or the dot notation).

See info '(guile) Association Lists'.

Thank you for the review!

45mg, would you mind sending a v4?

-- 
Maxim

Previous Next

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