Bug 1369008

confirmed with:
NetworkManager -V
1.0.6-27.el7
uname -r
3.10.0-327.el7.x86_64
where Master in slaves have the UUID of the bond master

the master can be either specified to be the name of an interface or it can be the UUID of another connection. Both is valid from NetworkManager's point of view.

nmtui currently only supports the latter.

It's a valid feature request to also support specifying the interface name. But note that nmtui by design does not support all options to keep the UI simpler. In those cases, nmcli is the expert tool that gives access to every option.

Created attachment 1225286 [details]
Attempt at producing more compatible ifcfg files

there is svUnsetValue(,) for svSetValueString(,,NULL)


Also, pre-setting all values to NULL and only setting them later is not nice. It's better unset values if-and-only-if they are not to be set.

E.g.:

   #the master property
   MASTER=bond0

   TYPE=Ethernet

results on update in:

   #the master property

   TYPE=Ethernet
   MASTER=bond1

if you only reset the value without clearing it you get:


   #the master property
   MASTER=bond1

   TYPE=Ethernet


Also, unsetting the value unnecessarily, always marks the ifcfg-file as dirty, even if there is no change.

(In reply to Thomas Haller from comment #5)
> 
> Also, pre-setting all values to NULL and only setting them later is not
> nice. It's better unset values if-and-only-if they are not to be set.
> 
> ...
>
> Also, unsetting the value unnecessarily, always marks the ifcfg-file as
> dirty, even if there is no change.

hm, rethinking this, shvarFile should become more intelligent and handle such cases automatically. It could do so by keeping a backup copy of the originally read values and then:

  - only actually store the file to disk if it really changed (maybe optionally,
    by having a always_write option).
  - it could avoid the reordering issue by merging the changes with the backup 
    copy.
  - it could even remove all keys that were not explicitly touched. Thus, making 
    the shUnsetValue() calls uneccessary because it would drop all keys that
    were not explicitly set. Preferably, not dropping them, but commenting them 
    out.

https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/ifcfg-device-rh1369008

Pushed some fixups.

Hoping you're *not* actually expecting me to refactor shfile.

(In reply to Lubomir Rintel from comment #7)
> 
> Hoping you're *not* actually expecting me to refactor shfile.

"refactoring" is the wrong word. Extend and improve.

Similar issues are all over the place, e.g. https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c?id=a96c819f6f5d1c4faa21b242bea18096049c9674#n1318

~somebody~ should fix it and it's rather simple.

Merged upstream in 80e43390310c32462912eb88da2a9a12c2f29dc7

Turns out nmtui creates the slave connections first and then the master; thus the ifcfg plugin is unable to determine the master device name. This branch commits the slave connection only after the master editor is closed:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/nmtui-slaves-after-master-rh1369008

I would just remove the comment here:

--- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-connection.c
+++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-connection.c
@@ -340,10 +340,6 @@ commit_changes (NMSettingsConnection *connection,
        char *ifcfg_path = NULL;
        const char *filename;
 
-       /* To ensure we don't rewrite files that are only changed from other
-        * processes on-disk, read the existing connection back in and only rewrite
-        * it if it's really changed.
-        */
        filename = nm_settings_connection_get_filename (connection);
        if (filename) {
                success = writer_update_connection (NM_CONNECTION (connection),

as the connection is no more read back here.


Then found a minor typo in a comment:
--- a/clients/tui/nmt-editor-page.c
+++ b/clients/tui/nmt-editor-page.c
@@ -116,8 +116,8 @@ nmt_editor_page_add_section (NmtEditorPage *page,
  * nmt_editor_page_saved:
  * @page: the #NmtEditorPage
  *
- * This method is called when the user saves the connection. It givesa the
- * page a chance to do save its data outside the connections (such as
+ * This method is called when the user saves the connection. It gives
+ * the page a chance to save its data outside the connections (such as
  * recommit the slave connections).
  */

Branch lgtm.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2299