I don't think that is overly useful.

Specifying invalid setting plugins is a serious configuration error and I feel it's correct for NM to abort.

Note, that the available plugins are determined at compile time (or by your distribution who provides the binary package). Hence, usually it is very uncommon to ever change the main.plugins setting.

Created attachment 288663 [details] [review]
config: set a compile time default for the main.plugins configuration option

In case of a missing NetworkManager.conf (or a missing configuration option
main.plugins), allow to determine the fallback at compile time

https://bugzilla.gnome.org/show_bug.cgi?id=738611

Signed-off-by: Thomas Haller <thaller@redhat.com>

(In reply to comment #2)
> Created an attachment (id=288663) [details] [review]
> config: set a compile time default for the main.plugins configuration option

Btw. we always compile ibft plugin, but by default we don't enable it
in contrib/fedora/rpm/NetworkManager.conf . I think we should. (?)


In that case, maybe it would be right to:
+    #if CONFIG_PLUGIN_IFCFG_RH
+                                     "ifcfg-rh",
+                                     "ibft",
+    #endif

Thanks

Well, in our concrete case we used to use "ifnet" plugin but, now, we want people to keep with default "keyfile" then, we disable ifnet, don't install the networkmanager settings file but some people still have one they edited time ago and need to manually drop the line in their /etc file. We inform people about this... but still some forget it :|

(In reply to comment #4)
> Thanks
> 
> Well, in our concrete case we used to use "ifnet" plugin but, now, we want
> people to keep with default "keyfile" then, we disable ifnet, don't install the
> networkmanager settings file but some people still have one they edited time
> ago and need to manually drop the line in their /etc file. We inform people
> about this... but still some forget it :|

With the previous patch, you could (in the future) compile NM with a default, and not set the configuration value in the provided /etc/NetworkManager/NetworkManager.conf.
Maybe even better, to provide the default configuration file that contains:

[main]
# by default, NetworkManager uses the 'keyfile' plugin
#plugins=keyfile


Also, for your case you could just patch those few lines yourself before packaging. If that is really what you want to do.

To me, this seems the wrong upstream behaviour... NM should rather not be resilient to configuration errors. But yeah, I see that this is problematic -- especially for non-technical users.

The patch looks fine to me, and I think (at least for this option) it's useful to allow the fallback to happen, with a warning that the configured plugin can't be found.  For other config options I think we should leave it as a hard error.

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=288663) [details] [review] [details] [review]
> > config: set a compile time default for the main.plugins configuration option
> 
> Btw. we always compile ibft plugin, but by default we don't enable it
> in contrib/fedora/rpm/NetworkManager.conf . I think we should. (?)
> 
> 
> In that case, maybe it would be right to:
> +    #if CONFIG_PLUGIN_IFCFG_RH
> +                                     "ifcfg-rh",
> +                                     "ibft",
> +    #endif

Yes.

Ok, I changed it to only warn on missing plugins.

I reworked the previous patch. I pushed the new version to the th/bgo738611_load_setting_plugins branch.

> settings: accept missing settings plugins

> 		/* ifcfg-fedora was renamed ifcfg-rh; handle old configs here */
>-		if (!strcmp (pname, "ifcfg-fedora"))
>-			pname = "ifcfg-rh";
>+		if (!strcmp (pname, "ifcfg-fedora")) {
>+			g_free (pname);
>+			pname = g_strdup ("ifcfg-rh");
>+		}

That rename happened in NM 0.7, 6 years ago. We can probably drop the special case now...

> settings: check file permissions when loading settings plugins

The check here is the same one that nm-manager.c does for device plugins. Maybe it should be in NetworkManagerUtils.

(In reply to comment #8)
> > settings: accept missing settings plugins
> 
> > 		/* ifcfg-fedora was renamed ifcfg-rh; handle old configs here */
> >-		if (!strcmp (pname, "ifcfg-fedora"))
> >-			pname = "ifcfg-rh";
> >+		if (!strcmp (pname, "ifcfg-fedora")) {
> >+			g_free (pname);
> >+			pname = g_strdup ("ifcfg-rh");
> >+		}
> 
> That rename happened in NM 0.7, 6 years ago. We can probably drop the special
> case now...

fixup pushed.

> 
> > settings: check file permissions when loading settings plugins
> 
> The check here is the same one that nm-manager.c does for device plugins. Maybe
> it should be in NetworkManagerUtils.

Indeed I copied the code over from nm-manager.c But there is not so much overlapping code as the behaviour is slightly different. 

E.g. on "if (!S_ISREG (st.st_mode))" we want to silently skip loading device plugins (which come from a readdir()), but warn for setting plugins (because they were explicitly configured).

> settings: accept missing settings plugins

+#define LOG(level, ...) \
+	G_STMT_START { \
+		nm_log ((level), LOGD_CORE, \
+		        "setting: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__) \
+		        _NM_UTILS_MACRO_REST(__VA_ARGS__)); \
+	} G_STMT_END

-> "settings: "

merged to master:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=069fcf4ba37a5a6688d9b344a273a71177846358

Could it be included for 0.9.10.x branch too please (for getting it in an hypothetical upcoming 0.9.10.1 version) :)

Thanks a lot

(In reply to comment #12)
> Could it be included for 0.9.10.x branch too please (for getting it in an
> hypothetical upcoming 0.9.10.1 version) :)
> 
> Thanks a lot

Sure.

Backported to nm-0-9-10:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=3d9c8f6b8dd395177063577ce32ff2cabf23cef9