Subject: Re: [PATCH v3] services: network-manager: Add extra-configuration-files field.

From: Maxim Cournoyer <[email-masked]>
Date: Sun, 02 Feb 2025 15:46:57 +0900

Hi Arnaud,

Arnaud Daby-Seesaram <[email-masked]> writes:

> Hi,
>
> Thank you for the v3.  I just have two minor comments.
>
>
> 45mg <[email-masked]> 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