GNU bug report logs - #76688
[PATCH] services: network-manager: Handle existing configuration directory.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 76688 in the body.
You can then email your comments to 76688 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: 45mg <45mg.writes <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: 45mg <45mg.writes <at> gmail.com>
Subject: [PATCH] services: network-manager: Handle existing configuration
 directory.
Date: Sun,  2 Mar 2025 23:59:49 +0530

* gnu/services/networking.scm (network-manager-activation):
Handle the case where /etc/NetworkManager/conf.d already exists.

Change-Id: I7af4f4ad226eef28bd8667f0708525e77a6a50c8
---
Follow-up to 0caba8f5db48c15a2c3edae37e816654246fa986.

This issue only became apparent after pulling the above commit, changing my
system config to use the new extra-files field, and reconfiguring /twice/. This
sort of thing is why I now pull from a local fork and apply patches to it before
submitting them ;)

 gnu/services/networking.scm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
index 53840c2764..45efea330b 100644
--- a/gnu/services/networking.scm
+++ b/gnu/services/networking.scm
@@ -1271,7 +1271,18 @@ (define (network-manager-activation config)
                '((mkdir-p "/var/lib/misc"))
                '())
         #$@(if (pair? extra-configuration-files)  ;if non-empty
-               `((symlink
+               ;; If /etc/NetworkManager/conf.d is a symlink to a store file,
+               ;; delete it.
+               `((if (and (file-exists? "/etc/NetworkManager/conf.d")
+                          (store-file-name?
+                           (canonicalize-path "/etc/NetworkManager/conf.d")))
+                     (delete-file-recursively
+                      "/etc/NetworkManager/conf.d"))
+                 ;; If it exists but is not a symlink to a store file, then
+                 ;; this will fail with EEXIST; we leave this for the user to
+                 ;; handle, since they probably created the directory
+                 ;; themselves.
+                 (symlink
                   ,(file-union "network-manager-configuration-directory"
                                extra-configuration-files)
                   "/etc/NetworkManager/conf.d"))

base-commit: f9dcb84550b85aa816899b2106b1a5ae546167a3
-- 
2.48.1

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

From: Arnaud Daby-Seesaram <ds-ac <at> nanein.fr>
To: 45mg <45mg.writes <at> gmail.com>
Cc: 76688 <at> debbugs.gnu.org
Subject: Re: [bug#76688] [PATCH] services: network-manager: Handle existing
 configuration directory.
Date: Fri, 07 Mar 2025 08:57:07 +0100

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

Hi,

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

> * gnu/services/networking.scm (network-manager-activation):
> Handle the case where /etc/NetworkManager/conf.d already exists.
>
> Change-Id: I7af4f4ad226eef28bd8667f0708525e77a6a50c8
> ---
> Follow-up to 0caba8f5db48c15a2c3edae37e816654246fa986.

This patch looks good to me.


Best regards,

-- 
Arnaud

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

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

From: 45mg <45mg.writes <at> gmail.com>
To: Arnaud Daby-Seesaram <ds-ac <at> nanein.fr>, 45mg <45mg.writes <at> gmail.com>
Cc: 76688 <at> debbugs.gnu.org
Subject: Re: [bug#76688] [PATCH] services: network-manager: Handle existing
 configuration directory.
Date: Fri, 07 Mar 2025 08:24:17 +0000

Hello again Arnaud!

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

> This patch looks good to me.

If it's not too much trouble, could you formally mark this patch as
reviewed? I think you can use the 'Prepare review' button on this page,
just check off the items and it'll automatically write the email for
you:

https://qa.guix.gnu.org/issue/76688

That way, it'll show up as 'reviewed looks good' in QA (dark green tick
icon). My understanding is that committers use that status to find
patches to apply.

Thanks again for reviewing this patch, as well as the last one!

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

From: Arnaud Daby-Seesaram <ds-ac <at> nanein.fr>
To: 45mg <45mg.writes <at> gmail.com>
Cc: 76688 <at> debbugs.gnu.org
Subject: Re: [bug#76688] [PATCH] services: network-manager: Handle existing
 configuration directory.
Date: Fri, 07 Mar 2025 10:36:27 +0100

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

user guix
usertag 76688 + reviewed-looks-good
thanks

Guix QA review form submission:
I am not sure about the commit message, otherwise, the patch looks good.

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

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

From: Vagrant Cascadian <vagrant <at> debian.org>
To: 45mg <45mg.writes <at> gmail.com>, 76688 <at> debbugs.gnu.org
Cc: Arnaud Daby-Seesaram <ds-ac <at> nanein.fr>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: Re: [bug#76688] [PATCH] services: network-manager: Handle existing
 configuration directory.
Date: Fri, 21 Mar 2025 23:38:49 -0700

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

On 2025-03-02, 45mg wrote:
> * gnu/services/networking.scm (network-manager-activation):
> Handle the case where /etc/NetworkManager/conf.d already exists.
>
> Change-Id: I7af4f4ad226eef28bd8667f0708525e77a6a50c8
> ---
> Follow-up to 0caba8f5db48c15a2c3edae37e816654246fa986.
>
> This issue only became apparent after pulling the above commit, changing my
> system config to use the new extra-files field, and reconfiguring /twice/. This
> sort of thing is why I now pull from a local fork and apply patches to it before
> submitting them ;)
>
>  gnu/services/networking.scm | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
> index 53840c2764..45efea330b 100644
> --- a/gnu/services/networking.scm
> +++ b/gnu/services/networking.scm
> @@ -1271,7 +1271,18 @@ (define (network-manager-activation config)
>                 '((mkdir-p "/var/lib/misc"))
>                 '())
>          #$@(if (pair? extra-configuration-files)  ;if non-empty
> -               `((symlink
> +               ;; If /etc/NetworkManager/conf.d is a symlink to a store file,
> +               ;; delete it.
> +               `((if (and (file-exists? "/etc/NetworkManager/conf.d")
> +                          (store-file-name?
> +                           (canonicalize-path "/etc/NetworkManager/conf.d")))
> +                     (delete-file-recursively
> +                      "/etc/NetworkManager/conf.d"))
> +                 ;; If it exists but is not a symlink to a store file, then
> +                 ;; this will fail with EEXIST; we leave this for the user to
> +                 ;; handle, since they probably created the directory
> +                 ;; themselves.
> +                 (symlink
>                    ,(file-union "network-manager-configuration-directory"
>                                 extra-configuration-files)
>                    "/etc/NetworkManager/conf.d"))

I hit this bug as well, and it actually caused a boot failure, had to
boot to an older generation to get things working again!

Have not yet tried the patch, though it looks like it should fix this
exact problem...

But... I am wondering if it wouldn't be more elegant to avoid using
/etc/NetworkManager/conf.d entirely, instead call NetworkManager
pointing the --config-dir arguments directly at the relevent store
directory, much like the configuration is passed using the --config
argument. That way there would be no leftovers between different system
generations...

Alternately, /run/NetworkManager/conf.d/*.conf should also be a possibly
better option than /etc/, although you still will have to clean up when
switching generations...

live well,
  vagrant

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

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Vagrant Cascadian <vagrant <at> debian.org>
Cc: 76688 <at> debbugs.gnu.org, Arnaud Daby-Seesaram <ds-ac <at> nanein.fr>,
 45mg <45mg.writes <at> gmail.com>
Subject: Re: [bug#76688] [PATCH] services: network-manager: Handle existing
 configuration directory.
Date: Sat, 22 Mar 2025 19:29:16 +0900

Hi Vagrant,

Vagrant Cascadian <vagrant <at> debian.org> writes:

[...]

> I hit this bug as well, and it actually caused a boot failure, had to
> boot to an older generation to get things working again!
>
> Have not yet tried the patch, though it looks like it should fix this
> exact problem...
>
> But... I am wondering if it wouldn't be more elegant to avoid using
> /etc/NetworkManager/conf.d entirely, instead call NetworkManager
> pointing the --config-dir arguments directly at the relevent store
> directory, much like the configuration is passed using the --config
> argument. That way there would be no leftovers between different system
> generations...

The thing is that NetworkManager, among other tools, make use of inotify
to monitor configuration changes, which is very useful as it avoids the
user having to restart the daemon or worst, reboot to have the changes
effected post a reconfiguration.  On such example was udev; see commit
e9fa17eb98efbd6211ab44ab49b8c078d4b73e04 that changed it from using a
store value for its config file to a /etc/udev/rules.d fixed location.

-- 
Thanks,
Maxim

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

From: Vagrant Cascadian <vagrant <at> debian.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 76688 <at> debbugs.gnu.org, Arnaud Daby-Seesaram <ds-ac <at> nanein.fr>,
 45mg <45mg.writes <at> gmail.com>
Subject: Re: [bug#76688] [PATCH] services: network-manager: Handle existing
 configuration directory.
Date: Sat, 22 Mar 2025 09:52:32 -0700

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

On 2025-03-22, Maxim Cournoyer wrote:
> Vagrant Cascadian <vagrant <at> debian.org> writes:
>> I hit this bug as well, and it actually caused a boot failure, had to
>> boot to an older generation to get things working again!
>>
>> Have not yet tried the patch, though it looks like it should fix this
>> exact problem...

I did get a chance to test the patch, and it worked to allow me to
reconfigure multiple times and still be able to reboot into the new
generation.

Though I did notice that nothing seems to clean up
/etc/NetworkManager/conf.d if you switch to a generation where the
configuration file is not present, so if you want to remove the
configuration you need to do so manually, which seems to break the
delcarative system configuration a bit.


>> But... I am wondering if it wouldn't be more elegant to avoid using
>> /etc/NetworkManager/conf.d entirely, instead call NetworkManager
>> pointing the --config-dir arguments directly at the relevent store
>> directory, much like the configuration is passed using the --config
>> argument. That way there would be no leftovers between different system
>> generations...
>
> The thing is that NetworkManager, among other tools, make use of inotify
> to monitor configuration changes, which is very useful as it avoids the
> user having to restart the daemon or worst, reboot to have the changes
> effected post a reconfiguration.  On such example was udev; see commit
> e9fa17eb98efbd6211ab44ab49b8c078d4b73e04 that changed it from using a
> store value for its config file to a /etc/udev/rules.d fixed location.

Fair! Though the conf.d symlink just points to the store (at least in
the configuration I tried) or, or will it notice when the symlink
changes? Are there cases where it would change from a single conf.d
symlink to a conf.d directory with multiple symlinks into the store? (so
far, I only tested with a single configuration file)

Regardless, I still wonder if it might be worth using
/run/NetworkManager/conf.d instead of /etc as this would at least get
automatically cleaned up between reboots.

live well,
  vagrant

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

Message #28 received at 76688-done <at> debbugs.gnu.org (full text, mbox):

From: Vagrant Cascadian <vagrant <at> debian.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 76688-done <at> debbugs.gnu.org, Arnaud Daby-Seesaram <ds-ac <at> nanein.fr>,
 45mg <45mg.writes <at> gmail.com>
Subject: Re: [bug#76688] [PATCH] services: network-manager: Handle existing
 configuration directory.
Date: Sun, 23 Mar 2025 10:33:05 -0700

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

On 2025-03-22, Vagrant Cascadian wrote:
> On 2025-03-22, Maxim Cournoyer wrote:
>> Vagrant Cascadian <vagrant <at> debian.org> writes:
>>> I hit this bug as well, and it actually caused a boot failure, had to
>>> boot to an older generation to get things working again!
>>>
>>> Have not yet tried the patch, though it looks like it should fix this
>>> exact problem...
>
> I did get a chance to test the patch, and it worked to allow me to
> reconfigure multiple times and still be able to reboot into the new
> generation.

Given that the patch is a few weeks old, and it is a relatively new
feature, and not having it can trigger a boot failure, I went ahead and
pushed as a1e87abaa364f8391cfd1f49bb01876f7a730bfb services:
network-manager: Handle existing configuration directory.

live well,
  vagrant

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

Message #31 received at 76688-done <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Vagrant Cascadian <vagrant <at> debian.org>
Cc: 76688-done <at> debbugs.gnu.org, Arnaud Daby-Seesaram <ds-ac <at> nanein.fr>,
 45mg <45mg.writes <at> gmail.com>
Subject: Re: [bug#76688] [PATCH] services: network-manager: Handle existing
 configuration directory.
Date: Mon, 24 Mar 2025 09:25:44 +0900

Hi Vagrant,

Vagrant Cascadian <vagrant <at> debian.org> writes:

> On 2025-03-22, Vagrant Cascadian wrote:
>> On 2025-03-22, Maxim Cournoyer wrote:
>>> Vagrant Cascadian <vagrant <at> debian.org> writes:
>>>> I hit this bug as well, and it actually caused a boot failure, had to
>>>> boot to an older generation to get things working again!
>>>>
>>>> Have not yet tried the patch, though it looks like it should fix this
>>>> exact problem...
>>
>> I did get a chance to test the patch, and it worked to allow me to
>> reconfigure multiple times and still be able to reboot into the new
>> generation.
>
> Given that the patch is a few weeks old, and it is a relatively new
> feature, and not having it can trigger a boot failure, I went ahead and
> pushed as a1e87abaa364f8391cfd1f49bb01876f7a730bfb services:
> network-manager: Handle existing configuration directory.

Thank you!

-- 
Maxim

Previous Next

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