GNOME Bugzilla – Bug 735255
NM exports invalid connections over DBUS without settings that are all-default
Last modified: 2014-09-19 18:57:15 UTC
Since merging bug 696936 (http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=fdef81207f7158a9acbdf900c05367bb960e6533), nmcli warns for me with: (process:9361): libnm-WARNING **: replace_settings: error updating connection /org/freedesktop/NetworkManager/Settings/21 settings: (5) connection.slave-type: slave-type 'bridge' requires a 'bridge-port' setting in the connection that happens due to my "virbr0-nic" device. NM assumes the following connection: [connection] id : "virbr0-nic" (s) uuid : "c6ab301b-3bec-459a-873e-3b9c402b51ae" (s) interface-name : "virbr0-nic" (s) type : "generic" (s) permissions : (sd) autoconnect : FALSE (s) timestamp : 1408752179 (s) read-only : FALSE (sd) zone : NULL (sd) master : "virbr0" (s) slave-type : "bridge" (s) secondaries : (sd) gateway-ping-timeout : 0 (sd) [bridge-port] priority : 32 (sd) path-cost : 100 (sd) hairpin-mode : FALSE (sd) [generic] Via DBUS only the following gets exported: {'connection': {'autoconnect': False, 'id': 'virbr0-nic', 'interface-name': 'virbr0-nic', 'master': 'virbr0', 'slave-type': 'bridge', 'timestamp': 1408752179L, 'type': 'generic', 'uuid': 'c6ab301b-3bec-459a-873e-3b9c402b51ae'}, 'generic': {}} The problem is in \ impl_settings_connection_get_settings() \ nm_connection_to_hash() \ nm_setting_to_hash() which skips NMSettingBridgePort because all properties are default. see [1] nm_setting_to_hash(): »···/* Don't return empty hashes, except for base types */ »···if (g_hash_table_size (hash) < 1 && !_nm_setting_is_base_type (setting)) { »···»···g_hash_table_destroy (hash); »···»···hash = NULL; »···} I think the solution is to drop check [1] and always return empty hashes. See also: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=4d32618264d7ca169036347a25f38689cce632f3 http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ad56cfa914d6a0f49b51e917ce89001f19c72d9f
Created attachment 284337 [details] [review] libnm: handle all-default settings in nm_setting_to_hash() properly Before, nm_setting_to_hash() would return NULL instead of an empty hash. This would be the case, if all properties are default. When exporting connections via DBUS, we eventually call nm_setting_to_hash() to convert the connection in a hash of hashes. By nm_setting_to_hash() converting empty hashes to NULL, the setting is missing. Not returning empty hashes means that to_hash() and new_from_hash() don't make a valid rount-trip conversion. Fix that by always returning a hash from nm_setting_to_hash() https://bugzilla.gnome.org/show_bug.cgi?id=735255 See-also: 4d32618264d7ca169036347a25f38689cce632f3 Signed-off-by: Thomas Haller <thaller@redhat.com>
Comment on attachment 284337 [details] [review] libnm: handle all-default settings in nm_setting_to_hash() properly I assume you did a full "make check" after this? This seems like probably the right thing, but wait until dcbw has had a chance to look at it; he may remember things this would break. (In particular, the fact that I didn't do the NMSettingGeneric fix (4d326182) this way makes me suspect he pointed out some reason not to...)
(In reply to comment #0) > Since merging bug 696936 > (http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=fdef81207f7158a9acbdf900c05367bb960e6533), > nmcli warns for me with: > > (process:9361): libnm-WARNING **: replace_settings: error updating connection > /org/freedesktop/NetworkManager/Settings/21 settings: (5) > connection.slave-type: slave-type 'bridge' requires a 'bridge-port' setting in > the connection > I see these ugly warnings too. The fix looks good, but let's wait for dcbw opinion.
I think the reason was mostly for secrets. When a client requests secrets, NM will hash the connection with the secrets, and send that back. Then the client calls nm_connection_update_secrets() with the hash. Previously, the only contents of the hash would be settings with secrets. Now, there will be all settings represented, even if they don't have any properties inside. We could run into the code in nm_connection_update_secrets() that has this comment: /* check first, whether all the settings exist... */ since if now NetworkManager is using libnm, but the client using libnm-util, they may not agree on whether or not the connection has all the settings including defaults. Other than that potential issue (which I don't think is very important), I think we're OK. The only other thing that cares would be nm_setting_new_from_hash(), but that creates the new setting anyway and sets default values. So I'm tempted to +1 this patch and just see if we run into any issues in development.
we could also make nm_connection_to_dbus() still strip empty settings if (flags & ONLY_SECRETS)
Attachment 284337 [details] pushed as 6325c59 - libnm: handle all-default settings in nm_setting_to_hash() properly