GNOME Bugzilla – Bug 740738
[th/keyfile-write-connection-bgo740738] review: renaming a connection doesn't result in renaming the file
Last modified: 2015-07-24 09:34:44 UTC
I used nm-connection-editor (that shouldn't make any change, right?) to create and then rename a connection. NetworkManager created the connection file in /etc/NetworkManager/system-connections with the correct name initially, but didn't rename the file to the new name. That way the administrator can't easily find the file and other issues may possibly occur.
> I used nm-connection-editor (that shouldn't make any change, right?) (-- I think, that makes no difference.) I agree, keyfile should rename the files if the connection.id changes. -- As long, as no such file already exists. If a file already exists, we probably should perform no renaming at all. Otherwise it'd becomes a deep rabbit hole of renaming.
I would actually be happy if we could enforce the identity of the name and ID but I don't know whether that's feasible. In that case all files would be named correctly no id would be needed (if configured by hand) and any renaming would be possible in a consistent state, in an inconsistent one, you could simply refuse to save the connection under the new name (looking at it from the perspective of nm-connection-editor).
We don't (currently) enforce uniqueness of connection IDs, so we couldn't enforce a 1-1 mapping of ID to name.
OK, then treat the bug report just as a suggestion and change it to whatever is the best we can do or close it.
Created attachment 291740 [details] [review] keyfile: rename files when changing connection id Only take this endeavor when no conflicting keyfile exists. Otherwise, keep the old filename. If the desired name exists, we also try ID-UUID as second guess.
Created attachment 291838 [details] [review] keyfile: rename files when changing connection id Only take this endeavor when no conflicting keyfile exists. Otherwise, keep the old filename. If the desired name exists, we also try ID-UUID as second guess. This basically undoes commit eeb19fe2164ab97d84c0d46b978e91768cdb1878, which was added to fix the related bug #682570.
(In reply to comment #6) Note that in bug 682570 the main issue was that connections were duplicated when changing the connection ID. That is already fixed by calling unlink(existing_path). But patch eeb19fe2164ab97d84c0d46b978e91768cdb1878 also disables the renaming of keyfiles. Let's undo that patch.
(In reply to comment #6) > Created an attachment (id=291838) [details] [review] > keyfile: rename files when changing connection id > > Only take this endeavor when no conflicting keyfile > exists. Otherwise, keep the old filename. > > If the desired name exists, we also try ID-UUID as second > guess. > > This basically undoes commit eeb19fe2164ab97d84c0d46b978e91768cdb1878, > which was added to fix the related bug #682570. existing_path will never be outside of keyfile_dir since it is always based on keyfile_dir. That can just be enforced with g_return_val_if_fail() or even g_assert(), though we have to be careful of testcases here. So we don't need that part of the check in the first hunk of the patch. existing_path should only be used as a fallback, since it's just the cached path of the last place the file was saved. If we always rename to the current ID instead of preferring existing_path, then the only time we use existing_path is if all renaming efforts failed. So we never would "prefer" existing_path at all since it would be used as a last resort.
Created attachment 291936 [details] [review] keyfile: rename files when changing connection id Only take this endeavor when no conflicting keyfile exists. Otherwise, keep the old filename. If the desired name exists, we also try ID-UUID as second guess. This basically undoes commit eeb19fe2164ab97d84c0d46b978e91768cdb1878, which was added to fix the related bug #682570.
Created attachment 291937 [details] [review] keyfile: retry finding a suitable filename in writer() Try harder to find a suitable filename by appending a counter to the name.
jklimes, any comment about these patches WRT https://bugzilla.gnome.org/show_bug.cgi?id=682570#c2 ? Bug 682570 and this bug are requesting exactly opposite behavior WRT file name matching the ID.
(In reply to comment #11) > jklimes, any comment about these patches WRT > https://bugzilla.gnome.org/show_bug.cgi?id=682570#c2 ? Bug 682570 and this bug > are requesting exactly opposite behavior WRT file name matching the ID. I still prefer not renaming files if ID changes. It seems nice if the connection name and the file name match. On the other hand I see file name changes quite unfriendly, because I would expect that the file name is not mysteriously changed or renamed (namely if it is created manually).
On the other other hand, I tend to assume that if I have a connection named "foobar", that it's going to be in /etc/sysconfig/network-scripts/ifcfg-foobar, and I find it confusing when it's not. How about if we rename the file if its old filename matches its old ID, but keep the old filename otherwise?
(In reply to comment #12) > (In reply to comment #11) > > jklimes, any comment about these patches WRT > > https://bugzilla.gnome.org/show_bug.cgi?id=682570#c2 ? Bug 682570 and this bug > > are requesting exactly opposite behavior WRT file name matching the ID. > > I still prefer not renaming files if ID changes. > It seems nice if the connection name and the file name match. On the other hand > I see file name changes quite unfriendly, because I would expect that the file > name is not mysteriously changed or renamed (namely if it is created manually). Even with this change we would not randomly rename files. They get only renamed during a save. I would very much expected that your file changes in that case. Including a rename. Note that this was the original behavior it got changed: https://bugzilla.gnome.org/show_bug.cgi?id=682570#c2 . This RFE requests again renaming and undo that change. I would agree with that later option. (In reply to comment #13) > On the other other hand, I tend to assume that if I have a connection named > "foobar", that it's going to be in /etc/sysconfig/network-scripts/ifcfg-foobar, > and I find it confusing when it's not. (btw. these patches are about keyfiles. But the same reasoning applies) > How about if we rename the file if its old filename matches its old ID, but > keep the old filename otherwise? But inside _internal_write_connection(), where the decision is made, the original connection-id is not easily at hand. And I would find this behavior inconsistent. I would either always or never rename. But yeah... how do we agree?
Created attachment 294161 [details] [review] keyfile: retry harder finding a suitable filename in writer() Try harder to find a suitable filename by appending a counter to the name.
Created attachment 294162 [details] [review] keyfile: rename files when changing connection id Only take this endeavor when no conflicting keyfile exists. Otherwise, keep the old filename. If the desired name exists, we also try ID-UUID as second guess. This basically undoes commit eeb19fe2164ab97d84c0d46b978e91768cdb1878, which was added to fix the related bug #682570.
Obsoleted previous patches. The first (attachment 294161 [details] [review]) does not change any behavior regarding rename or not. It only adds retrying to find a suitable file name. I think this one can be commited any case. The second is a trivial patch, whether we want to rename the file on rename or not.
Comment on attachment 294161 [details] [review] keyfile: retry harder finding a suitable filename in writer() http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=3b1c5ee0fde05f2d7d786d474591f4d23d64284a
Commited attachment 294161 [details] [review]. attachment 294162 [details] [review] is left undecided, and the question is *what* to do and whether to rename the file -- the "how" is trivial.
(In reply to comment #13) > On the other other hand, I tend to assume that if I have a connection named > "foobar", that it's going to be in /etc/sysconfig/network-scripts/ifcfg-foobar, > and I find it confusing when it's not. My feeling is that if you explicitly rename the connection through NM (which most people won't do, and most people on servers won't do either), then yeah, I think it should get renamed. We should obviously make sure it only happens when explicitly requested, of course (eg, NM should *not* rename when it reads a connection off disk where the ID != filename), and these patches do that AFAICT. > How about if we rename the file if its old filename matches its old ID, but > keep the old filename otherwise? I would agree with that, it sounds like a nice compromise.
New branch th/keyfile-write-connection-bgo740738 for review. - it adds a configuration option keyfile.rename-file which allows the user to configure that keyfile plugin will rename files. - also do some cleanup, fix a memleak, and improve logging and error messages.
> keyfile: add info logging when updating connection + nm_log_info (LOGD_SETTINGS, "keyfile: update "NM_KEYFILE_CONNECTION_LOG_FMT" and persistate connection", persist connection? IMO always renaming the file when keyfile.rename-file=yes even if the old filename is different from old ID makes sense. The branch looks good to me.
(In reply to Beniamino Galvani from comment #22) > > keyfile: add info logging when updating connection > > + nm_log_info (LOGD_SETTINGS, "keyfile: update > "NM_KEYFILE_CONNECTION_LOG_FMT" and persistate connection", > > persist connection? > > IMO always renaming the file when keyfile.rename-file=yes even if the old > filename is different from old ID makes sense. The branch looks good to me. Fixed && repushed.
> keyfile: add keyfile.rename-file config option Hmm, I don't think we need a config option for this. Was something not easily implemented about comment #20? The rest of the patches look OK though.
(In reply to Dan Williams from comment #24) > > keyfile: add keyfile.rename-file config option > > Hmm, I don't think we need a config option for this. Was something not > easily implemented about comment #20? I moved that commit as last, and added one fixup! commit. Repushed. How is it now?
Yeah, I like that a lot better. LGTM.
merged to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=be5f85ff637204e126346f71576fce53837405ac Closing BZ as fixed, because if you now modify the connection ID via D-Bus, the keyfile will be renamed (this only works for keyfile plugin).