(In reply to comment #0)
> Not related note:
> While testing this, I noticed that ethernet properties like 'speed', 'port',
> 'duplex' are note saved when using ifcfg-rh plugin.

Yeah, they actually never got implemented in NM, a lower priority I guess.  We'd need to use ethtool to set these since I don't think libnl can do it.

(In reply to comment #0)
> Maybe, I'm missing something. But when I want save/update connection with
> nm_remote_connection_commit_changes() it takes new settings out of the
> NMRemoteConnection (the first parameter). It is a problem because if I first
> save a connection via nm_remote_settings_add_connection(), a new
> NMRemoteConnection is created. On update I need to change this connection.

nm_remote_settings_add_connection() should wait until NetworkManager signals the new remote connection is ready and then call your callback with the new RemoteConnection; it'll also emit the new-connection signal when the connection is ready.  If not we should fix it.

After this happens, what needs to be updated in the new RemoteConnection?  Are there additional properties that need to be set that weren't part of the original connection data sent to nm_remote_settings_add_connection() that the new RemoteConnection was created for?

Perhaps I don't understand the actual problem... if any of this doesn't make sense please let me know.

> I guess the function should rather look like
> nm_remote_connection_commit_changes (NMRemoteConnection *self,
>                                      NMConnection *new_connection,
>                                      NMRemoteConnectionCommitFunc callback,
>                                      gpointer user_data)
> 
> 'self' should be updated according to 'new_connection' and then send it to NM
> for storage.
> or introduce such a function with a new name (nm_remote_connection_update())

There are really two ways to change the remote connection data:

1) construct a completely new NMConnection and call nm_connection_replace_settings (remote, new_connection);

2) manipulate settings and properties of the RemoteConnection directly through nm_connection_get_setting (NM_CONNECTION (remote), XXX) and g_object_set() some property of the setting

In both these cases, of course the change is only made locally to the RemoteConnection and then it needs to be sent to NetworkManager, which is what commit_changes() is for.

I think for long-running operations, #1 is better, because if something else changes the connection at any point, the RemoteConnection object will be refreshed by libnm-glib automatically.

But for a short operation that doesn't run a mainloop iteration, such as changing one or two properties, method #2 is quicker and more direct.

Your suggestion, which is perfectly valid, would flip this around so that #2 would then become:

c = nm_connection_duplicate (NM_CONNECTION (remote));
s = nm_connection_get_setting (c, xxxx);
g_object_set (s, "foobar", "some value");
nm_remote_connection_commit_commit_changes (remote, c, xxx);

so I think it's no gain either way we do it.

> At present, I use this code snippet:
> GHashTable *new_settings;
> new_settings = nm_connection_to_hash (connection, NM_SETTING_HASH_FLAG_ALL);
> nm_connection_replace_settings  (NM_CONNECTION (rc), new_settings, NULL);
> g_hash_table_destroy (new_settings);
> nm_remote_connection_commit_changes (rc, update_connection_guided_cb, NULL);

Yeah, and you're totally correct that this is ugly too.  We should make this better.  So how about, instead of changing the commit_changes() function, we add a new libnm-util function like:

nm_connection_replace_settings_from_connection (old, new);

like the attached patch?  Then you could:

nm_connection_replace_settings_from_connection (NM_CONNECTION (remote), connection);
nm_remote_connection_commit_changes (remote, xxx);

instead of having to deal with the hash table?  Untested patch attached.

Created attachment 239894 [details] [review]
Implement nm_connection_replace_settings_from_connection()

(In reply to comment #2)
> (In reply to comment #0)
> > Maybe, I'm missing something. But when I want save/update connection with
> > nm_remote_connection_commit_changes() it takes new settings out of the
> > NMRemoteConnection (the first parameter). It is a problem because if I first
> > save a connection via nm_remote_settings_add_connection(), a new
> > NMRemoteConnection is created. On update I need to change this connection.
> 
> nm_remote_settings_add_connection() should wait until NetworkManager signals
> the new remote connection is ready and then call your callback with the new
> RemoteConnection; it'll also emit the new-connection signal when the connection
> is ready.  If not we should fix it.
> 
This work fine.

> After this happens, what needs to be updated in the new RemoteConnection?  Are
> there additional properties that need to be set that weren't part of the
> original connection data sent to nm_remote_settings_add_connection() that the
> new RemoteConnection was created for?
> 
> Perhaps I don't understand the actual problem... if any of this doesn't make
> sense please let me know.
> 
I probably didn't express myself clearly and have different expectations of 
nm_remote_connection_commit_commit_changes().
I kind of guessed it would take a new connection and update both NMRemoteConnection locally and send it to NM.
I now realize that I should update NMRemoteConnection first myself and then simply commit the changes with nm_remote_connection_commit_commit_changes().

Your patch of nm_connection_replace_settings_from_connection() will help in no messing with hashes. See my changes to the patch bellow.

My basic use case is:
- creating NMConnection object
- save it the first time with nm_remote_settings_add_connection
- continue changing the NMConnection object
- send updated connection to NM

Review of attachment 239894 [details] [review]:

The patch works fine.

Just minor changes:
- moving nm_connection_replace_settings_from_connection() right after nm_connection_replace_settings()
- renaming 'new' parameter to 'new_connection' (possible clash with 'new' keyword)
- adding 'Since: 0.9.10'
- updating libnm-util.ver

Created attachment 241133 [details] [review]
[PATCH] minor changes to the previous patch

Changes pointed out in the previous comment.

I consolidated jklimes' patch into the first commit and then converted the only(!) relevant user of nm_connection_to_hash() to use the new API.

Also see the applet git dcbw/editor-save-simplify branch for another conversion to the new API.

(In reply to comment #1)
> (In reply to comment #0)
> > Not related note:
> > While testing this, I noticed that ethernet properties like 'speed', 'port',
> > 'duplex' are note saved when using ifcfg-rh plugin.
> 
> Yeah, they actually never got implemented in NM, a lower priority I guess. 
> We'd need to use ethtool to set these since I don't think libnl can do it.

Feel free to add a RFE and assign it to me if you have something related to nm-platform.

(In reply to comment #7)
> I consolidated jklimes' patch into the first commit and then converted the
> only(!) relevant user of nm_connection_to_hash() to use the new API.
> 
> Also see the applet git dcbw/editor-save-simplify branch for another conversion
> to the new API.
I just wonder if it is guaranteed that self->>orig_connection is NM_REMOTE_CONNECTION and not a plain NM_CONNECTION in 

nm_remote_connection_commit_changes (
  NM_REMOTE_CONNECTION (self->orig_connection),
  updated_connection_cb, self);

Otherwise, it seems good.

(In reply to comment #9)
> I just wonder if it is guaranteed that self->>orig_connection is
> NM_REMOTE_CONNECTION and not a plain NM_CONNECTION

It is; self->connection is a plain NMConnection, but if orig_connection is non-NULL, it's always an NMRemoteConnection.

So it all looks good?

Looks good to me.

Merged.

Created attachment 242133 [details] [review]
[PATCH] make nm_connection_replace_settings_from_connection() safer

Reopening to add a small fix to nm_connection_replace_settings_from_connection()

Handle connection==new_connection condition gracefully so that 'connection' is not harmed.

Comment on attachment 242133 [details] [review]
[PATCH] make nm_connection_replace_settings_from_connection() safer

Hm... should this be a silent no-op or a g_return_if_fail()? What circumstances does this happen in?

I think silent no-op; I don't know why a client would try this (maybe it just makes the code easier?) but it doesn't seem like we should penalize the client if the solution is so simple.

Pushed the patch, thanks!

Basically, it was just an client (my ;-)) error. But I think it is not nice to destroy the connection due to that.
Thanks!