You probably do want nm_remote_settings_reload_connections() to be an async call, because the error message might be important.  Only 'root' should be reloading connection files, really (maybe an argument for modify-system permission holders, but tenuous) and so you'll want to print out the error if the user isn't.

Should also note somewhere that this will blow away any unsaved connections, and resync them with what's on-disk.  New question: what happens in the future if we have an unsaved connection where the NMDevice has been updated with the connection details, and then somebody calls ReloadConnections?  Do we resync the device with the now-from-disk connection details and blow away the current live config?  We never quite got around to deciding once and for all if updating a connection created a new one (which is tricky to do in the code) or updated the active connection and marked it unsaved, which is what I think I was advocating for.  However this change+mark-unsaved approach the problem exposed here.

Alternatively, if the connection is unsaved, then maybe we *don't* reload that specific connection?  That would then suggest we also have a per-connection "reload" call I guess, otherwise you couldn't get back to the original on-disk state if the connection was unsaved.

Next, authentication.  If we accept that only root should do this call, then we need to enforce that.  It seems that D-BUs permissions files are on their way out, so we need to get the user UID from the DBusGMethodInvocation and check against 0, or do a full PolicyKit authentication and check for the modify-system permission.  I guess for now I'd just go with root and if anyone really needs to reload connections as a normal user later, tack it onto modify-system later.

+		if (reload) {
+			connection = g_hash_table_lookup (oldconns, full_path);
+			if (connection)
+				g_hash_table_remove (oldconns, full_path);
+		}

Wouldn't if (reload) g_hash_table_remove() be easier here?  If it's there, it gets removed, if not, nothing happens?

Any reason for ifcfg-rh to have the 'reload' argument for reload_connections()?  Shouldn't the behavior be the same in both cases?  If we're loading connections for the first time, then the old_connections hash should be empty and thus any of the stuff in if(reload) would be a NOP?

Also, I'm not sure the code for reloading for keyfile and ifcfg-rh is completely correct for connection renames, where the same UUID will now be a different path.  I think you can make that work just by using the UUID in old_conns isntead of the full path, and then looking up the connection by UUID instead of the full path.  Yeah that means reading and parsing everything, but if you're reloading, it's a very rare occurance then.

Some minor stuff:

1) Initialize oldconns to suppress compile warnings/errors:
settings/plugins/keyfile/plugin.c: In function 'read_connections':
settings/plugins/keyfile/plugin.c:366:24: error: 'oldconns' may be used 
 src/settings/plugins/ifcfg-rh/plugin.c
plugin.c: In function 'read_connections':
plugin.c:424:24: error: 'oldconns' may be used uninitialized in this function [-Werror=maybe-uninitialized]

diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c
index dee5103..51d7e19 100644
--- a/src/settings/plugins/ifcfg-rh/plugin.c
+++ b/src/settings/plugins/ifcfg-rh/plugin.c
@@ -369,7 +369,7 @@ read_connections (SCPluginIfcfg *plugin, gboolean reload)
 	GDir *dir;
 	GError *err = NULL;
 	const char *item;
-	GHashTable *oldconns;
+	GHashTable *oldconns = NULL;
 	GHashTableIter iter;
 	gpointer key, value;
 	NMIfcfgConnection *connection = NULL;
diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c
index 2db15b3..89fa030 100644
--- a/src/settings/plugins/keyfile/plugin.c
+++ b/src/settings/plugins/keyfile/plugin.c
@@ -309,7 +309,7 @@ read_connections (NMSystemConfigInterface *config, gboolean reload)
 	GDir *dir;
 	GError *error = NULL;
 	const char *item;
-	GHashTable *oldconns;
+	GHashTable *oldconns = NULL;
 	GHashTableIter iter;
 	gpointer data;
 
-- 

2) libnm-glib/nm-remote-settings.c
> + * @settings: the %NMRemoteSettings
Use # instead of %
+ * @settings: the #NMRemoteSettings

3) man/NetworkManager.conf.xml
   data/server.conf.in
ReloadConnections call name could be mentioned in the descriptions:
 "...when explicitly requested via ReloadConnections D-Bus call".

4) man/NetworkManager.conf.xml
>+	should set up file monitors so that changes made to
>+	connection files while NetworkManager is running will be
>+	picked up by NetworkManager.

I would either delimit the dependent clause with commas or move it to the end of the sentence
so that it is easier readable:
+	connection files, while NetworkManager is running, will be
+	picked up by NetworkManager.
or
+	connection files will be picked up by NetworkManager while NetworkManager is running

5) > g_warning ("Unrecognized value for main.monitor-connection-files: %s. Assuming 'false'", value);
Do we want to use nm_log_warn () instead?

6) add reload to usage() in nmcli
diff --git a/cli/src/connections.c b/cli/src/connections.c
index ee1796c..17c4a95 100644
--- a/cli/src/connections.c
+++ b/cli/src/connections.c
@@ -205,7 +205,8 @@ usage (void)
 #endif
 	         "  down [ id | uuid | path | apath ] <ID>\n\n"
 	         "  add COMMON_OPTIONS TYPE_SPECIFIC_OPTIONS IP_OPTIONS\n\n"
-	         "  delete [ id | uuid | path ] <ID>\n\n\n"
+	         "  delete [ id | uuid | path ] <ID>\n\n"
+	         "  reload\n\n\n"
 	         ));
 }
 
--

7) update nmcli manpage for 'reload'
see jklimes/nmcli-reload-man

I am still going to perform some tests.

(In reply to comment #1)
> You probably do want nm_remote_settings_reload_connections() to be an async
> call, because the error message might be important.

async, or just get rid of the _no_reply() and do it synchronously? It seems likely that only nmcli is ever going to be calling this, so we don't really need a non-blocking version do we?

> New question: what happens in the future
> if we have an unsaved connection where the NMDevice has been updated with the
> connection details, and then somebody calls ReloadConnections?

Your head asplode.

Of course, the same question applies without ReloadConnections too, whenever you edit the file.

> Alternatively, if the connection is unsaved, then maybe we *don't* reload that
> specific connection?  That would then suggest we also have a per-connection
> "reload" call I guess, otherwise you couldn't get back to the original on-disk
> state if the connection was unsaved.

We might want a SettingsConnection.Revert(), but that still leaves the question of whether you'd be reverting to the original in-memory state or the actual disk state...

> Next, authentication.  If we accept that only root should do this
> call, then we need to enforce that.

Yes. The files are only editable by root anyway, so there's no good argument for letting anyone else reload them.

> Wouldn't if (reload) g_hash_table_remove() be easier here?  If it's there, it
> gets removed, if not, nothing happens?

Oops, yeah, I copied that from the keyfile version, where there's more code inside the if body, and then wasn't paying enough attention when I removed that code here.

> Any reason for ifcfg-rh to have the 'reload' argument for
> reload_connections()?

No. I just assumed it was going to be necessary, but apparently it's not.

> Also, I'm not sure the code for reloading for keyfile and ifcfg-rh is
> completely correct for connection renames

Hm... yeah.

(In reply to comment #2)
> 5) > g_warning ("Unrecognized value for main.monitor-connection-files: %s.
> Assuming 'false'", value);
> Do we want to use nm_log_warn () instead?

nm_config_new() gets called before nm_logging_init(), so calling nm_log is bad because it will cause the syslog connection to be implicitly opened with the wrong options.

Although... maybe we should just make nm_log redirect to g_warning/g_message/g_debug if nm_logging_init() hasn't been called yet. That will work better when nm_log gets called from a test program as well... (The settings plugins also don't use nm_log, and I assume that this is part of the reason why; you don't want errors in /var/log/messages from running "make check".)

The plugins really, really need logging help, they never got ported over and so they have no facility to use warn/err/info levels or anything.  This was mainly becuase they need to prefix their messages with their plugin name for clarify, and we never got around to doing that.  We should convert over all the PLUGIN_WARN/PLUGIN_PRINT stuff there into some kind of more structured logging.

(In reply to comment #1)
> You probably do want nm_remote_settings_reload_connections() to be an async
> call, because the error message might be important.

Fixed

> Should also note somewhere that this will blow away any unsaved connections,
> and resync them with what's on-disk.

That applies to the normal automatic reloading too though, so I added the docs to AddUnsaved and UpdateUnsaved.

> Next, authentication.

done

> Also, I'm not sure the code for reloading for keyfile and ifcfg-rh is
> completely correct for connection renames

It was not. In fact, ifcfg-rh didn't correctly handle renames before either. Both of them should now.

Just looking at the last patch. You are changing 'connection' to 'connections' and 'device' to 'devices'. Given that we agreed on singular names and they were used for a long time, this looks like a request to change this policy. As such, it should be a separate and properly commented request!

I think the commit message explains the rationale. In particular, "nmcli connection reload" seemed weird to me because it suggests you're only reloading a single connection.

I guess the diff doesn't make this clear, but this doesn't change the help output; nmcli still claims that the subcommands are "connection" and "device" (as does all of the documentation). It's just that now, if you mistakenly type "connections" or "devices", it will accept those too.

(In reply to comment #8)
> I think the commit message explains the rationale. In particular, "nmcli
> connection reload" seemed weird to me because it suggests you're only reloading
> a single connection.

It's the same with iproute2 and many other tools. I don't see a reason to make distinction between 'connection add' and 'connection(s) reload' in CLI, just as I don't for 'ip route add' versus 'ip route(s) flush'.

> I guess the diff doesn't make this clear, but this doesn't change the help
> output; nmcli still claims that the subcommands are "connection" and "device"
> (as does all of the documentation). It's just that now, if you mistakenly type
> "connections" or "devices", it will accept those too.

And what about bash completion, how will it complete? Does the change have any other side effects?

> src/settings/plugins/ifcfg-rh/plugin.c:read_connections()

ifcfg-rh logs removed files: 
PLUGIN_PRINT (IFCFG_PLUGIN_NAME, "removed %s.", (char *)key);
keyfile could do that too.

src/settings/plugins/ifcfg-rh/plugin.c:read_connections()
NMIfcfgConnection *connection = NULL;
is not necessary here, just use
connection_new_or_changed (plugin, full_path, NULL, &old_path);

> only allow root to ReloadConnections

cli/src/connections.c:do_connection_reload()
error->code, error->message should be translated to nmcli ones. I guess at least
NM_REMOTE_SETTINGS_ERROR_SERVICE_UNAVAILABLE -> NMC_RESULT_ERROR_NM_NOT_RUNNING
other errors -> NMC_RESULT_ERROR_UNKNOWN

Being at that, a question to nmcli error codes:
Do you think, we should extend that for each new command or rather just have a generic one - COMMAND_FAILED (+ a few specific ones like NM_NOT_RUNNING,
USER_INPUT, TIMEOUT_EXPIRED, ...)?

> nmcli: accept "connections" and "devices"

I do not like this much. Basically, we have all commands in singular. I understand that if you read the commands as an english sentence it may sound weird, but I regard the command rather as an object nmcli works on.
Also, I doubt if it even helps because all docs advertize connection/device
and most users will just type "nmcli c r" or type "nmcli c<TAB>" and bash will complete 'connection'.

> testing revealed some issues too
1)
ifcfg-rh: renaming //etc/sysconfig/network-scripts/ifcfg-XXX1-16 -> //etc/sysconfig/network-scripts/ifcfg-XXX1-16
=>
Only should be done when old_path and path really differs.

2) run into an endless loop in ifcfg-rh when initially reading the connections
Debugging showed that there is a problem when two ifcfg files has the same UUID=
I had
'/etc/sysconfig/network-scripts/ifcfg-nag 4' and
'/etc/sysconfig/network-scripts/ifcfg-nag' with the same UUID=
=>
connection_new_or_changed() looped due to calling
g_object_set (existing, NM_IFCFG_CONNECTION_UNMANAGED, new_unmanaged, NULL);
at the end of the function. I'll post the stacktrace.

3) also saw an assertion
claim_connection: assertion `nm_connection_get_path (NM_CONNECTION (connection)) == NULL' failed

I haven't dig any deeper. But, if you need a help, feel free to tell me.

Created attachment 246621 [details]
Backtrace for the loop in 2) of the previous comment

(In reply to comment #10)
> ifcfg-rh logs removed files: 
> PLUGIN_PRINT (IFCFG_PLUGIN_NAME, "removed %s.", (char *)key);
> keyfile could do that too.

It does, it just does it from somewhere else... basically the way things are logged in the keyfile plugin required fewer changes than the way they are logged in ifcfg-rh, so you don't see as much logging-related stuff in the diff.

> NMIfcfgConnection *connection = NULL;
> is not necessary here, just use
> connection_new_or_changed (plugin, full_path, NULL, &old_path);

Hm... that's weird. I think connection is supposed to be set in some cases. This is probably related to the looping bug you found.

> Being at that, a question to nmcli error codes:
> Do you think, we should extend that for each new command or rather just have a
> generic one - COMMAND_FAILED (+ a few specific ones like NM_NOT_RUNNING,
> USER_INPUT, TIMEOUT_EXPIRED, ...)?

Not sure... I think if you want detailed error handling you should probably be using libnm-glib or the D-Bus interfaces directly.

> > nmcli: accept "connections" and "devices"
> 
> I do not like this much.

OK, since no one else likes it I'll remove that patch.

(In reply to comment #13)
> > NMIfcfgConnection *connection = NULL;
> > is not necessary here, just use
> > connection_new_or_changed (plugin, full_path, NULL, &old_path);
> 
> Hm... that's weird. I think connection is supposed to be set in some cases.
> This is probably related to the looping bug you found.

Not related to the looping, but it was causing another problem; it claimed every existing connection had been renamed.

I pushed two new patches; one to fix that, and one to fix the infinite loop. Plus removing the "nmcli connections" patch. I think that addresses everything?

Looks good to me now.

*** Bug 671752 has been marked as a duplicate of this bug. ***