GNOME Bugzilla – Bug 637825
NM should *not* store connnection timestamps in /etc/NetworkManager/system-connections/ - poor interaction with etckeeper, etc.
Last modified: 2011-03-08 10:00:06 UTC
NetworkManager seems to be updating the timestamp field in /etc/NetworkManager/system-connections/<connection-name> for an active connection every 5 minutes. This is rather undesirable for someone who uses etckeeper or any other system for tracking changes to configuration files in /etc, as it causes very many irrelevant changes. Granted, NM doesn't target servers, but even desktop users may desire to have a good idea what is going on in their /etc. I think NM should store the configuration in /etc, and store mutable state data relating to connections somewhere under /var.
The following patches implement a solution. Timestamps are stored to a database file (pure text file) /var/lib/NetworkManager/timestamps. That is updated. Connection configuration is not touched any more for timestamps. Dan, would you review?
Created attachment 181837 [details] [review] timestamp updating for system connections I've decided in the end not to update timestamp internally in connections property, because this saves us couple of nasty workarounds in plugins. The only thing that needs to be done is putting the right timestamp for GetSettings() D-Bus calls.
Created attachment 181838 [details] [review] get rid of LAST_CONNECT We don't need LAST_CONNECT in ifcfg-rh any more.
Looks good for 0.8.x, push it. For git master it'll need some adjustment due to changes in the source tree layout. But it can be a lot simpler on git master since since every exported connection is a subclass of NMSettingsConnection. So maybe we could get rid of update_active_connection_timestamp() in nm-manager.c and just call a new nm_settings_connection_update_timestamp (connection, guint64 ts) function that lives in src/settings/nm-settings-connection.c to update and save the timestamp to the keyfile. Then the code that needs to get run when the connection is removed can go into do_delete(). And we can make a new nm_settings_connection_read_timestamp() or something to read the timestamp in from disk which gets called from NMSettings' claim_connection() call. That way everything related to timestamps can be in nm-settings-connection.c. Do you think that might work?
Patches from comments #2 and #3 pushed to NM_0_8: 7d707bd2138561fc7b435719c0906b025f2075e2 33435b1872dc60c19657efb00343ed9665de5867
Dan, thanks for reviewing the patches and for suggestions to master branch patch. See that in the attachment and please review. Of course I'm also gonna to remove LAST_CONNECT from ifcfg-rh plugin (and adjust tests) the same way as for 0.8.x branch.
Created attachment 182256 [details] [review] new timestamp handling for master branch
Looks good; two comments: 1) you can use nm_connection_get_uuid() instead of the old s_con = nm_connection_get_setting (connection, NM_TYPE_SETTING_CONNECTION); g_assert (s_con); uuid = nm_setting_connection_get_timestamp(s_con); which saves a bit of code. I added that recently in 0.9. 2) Since all the timestamp code is now consolidated, I think you could make the timestamp a private variable in the NMSettingsConnectionPrivate structure instead of having to do the g_object_set_data()/g_object_get_data() stuff. It saves having to allocate the guint64* pointer too and makes things a bit clearer. I doesn't need to be a real GObject property, but just a member of the private struct that you can access with NM_SETTINGS_CONNECTION_GET_PRIVATE(). Other than that, looks good. Feel free to push when you've updated and set the bug to fixed, thanks!
Updated accordingly and pushed to master: 83d867796335b4d810f91448ca73e7e159d45545 Thanks a lot for the comments!