GNOME Bugzilla – Bug 737380
[review] add debugging function to print difference between NMConnection | th/bgo737380_log_connection_diff
Last modified: 2014-10-12 20:27:03 UTC
I got annoyed by not seeing the content of connections in the logfile. Added nm_utils_log_connection_diff(), which works mostly well. Note that for the commit "libnm-util: let nm_setting_diff() be symetric not to return properties..." we could actually fix this inconsistency in libnm-core (and change behavior). Does somebody know, why it was that way in the first place?
Branch name: th/bgo737380_log_connection_diff
> libnm-util: don't assert in nm_setting_get_secret_flags() and avoid assertion in agent_secrets_done_cb() Secret agents shouldn't be doing that, they should (at least for now) only return secrets in the hash. The D-Bus API docs are kinda clear, but possibly ambiguous: "Nested settings maps containing secrets. Each setting MUST contain at least the 'name' field, containing the name of the setting, and one or more secrets." However, since there are apparently some doing this, it makes sense to silently ignore the non-secret properties. For the comment: /* FIXME: NMSettingVPN:NM_SETTING_VPN_SECRETS has NM_SETTING_PARAM_SECRET... @#$%&! Yeah, it's got PARAM_SECRET because the whole has is secrets and thus clearly the parameter is secret too. I think that makes sense. That's one reason why nm_setting_get_secret_flags() even exists, so that the VPN setting can override it and return the correct secret. So yes, we can't do flags-based checking from nm-setting.c on NM_SETTING_VPN_SECRETS, but I think we need to return TRUE there instead of FALSE, because then nm_connection_diff() can continue with diffing the secrets, and it correctly handles the flags in nm-setting-vpn.c::compare_property(). > libnm-util: let nm_setting_diff() be symetric not to return properties that are set to default I think here, we should just leave libnm-util alone, and just change it in libnm-core to ignore non-default properties. Anyone using libnm-util would have worked around this issue if they cared, or filed a bug for it. We can still change libnm-core behavior to be "perfect" :) and I think we should. We just need to note this in any kind of migration docs. > libnm: add function _nm_setting_get_setting_priority() Any reason for the swap of args in the return of _nm_setting_type_is_base_type()? > core: add nm_utils_log_connection_diff struct _log_connection_setting_data We can just do "typedef struct { } LogConnectionSettingData" there to keep style with the rest of NM. I know that's not your preferred style, but lets try to keep it consistent... :) _log_connection_get_property() - the comment "/* for NULL, we don't want to return the string "NULL". */" doesn't make sense to me, because it looks like the code does exactly that and returns "NULL". Was the comment switched around?
(In reply to comment #2) > > libnm-util: don't assert in nm_setting_get_secret_flags() and avoid assertion in agent_secrets_done_cb() > > Secret agents shouldn't be doing that, they should (at least for now) only > return secrets in the hash. The D-Bus API docs are kinda clear, but possibly > ambiguous: "Nested settings maps containing secrets. Each setting MUST contain > at least the 'name' field, containing the name of the setting, and one or more > secrets." > > However, since there are apparently some doing this, it makes sense to silently > ignore the non-secret properties. > > For the comment: > > /* FIXME: NMSettingVPN:NM_SETTING_VPN_SECRETS has NM_SETTING_PARAM_SECRET... > @#$%&! > > Yeah, it's got PARAM_SECRET because the whole has is secrets and thus clearly > the parameter is secret too. I think that makes sense. That's one reason why > nm_setting_get_secret_flags() even exists, so that the VPN setting can override > it and return the correct secret. > > So yes, we can't do flags-based checking from nm-setting.c on > NM_SETTING_VPN_SECRETS, but I think we need to return TRUE there instead of > FALSE, because then nm_connection_diff() can continue with diffing the secrets, > and it correctly handles the flags in nm-setting-vpn.c::compare_property(). I reworked this commit, added a fixup!. Also, added new commit "libnm: hide API for generic handling of secrets" > > libnm-util: let nm_setting_diff() be symetric not to return properties that are set to default > > I think here, we should just leave libnm-util alone, and just change it in > libnm-core to ignore non-default properties. Anyone using libnm-util would > have worked around this issue if they cared, or filed a bug for it. We can > still change libnm-core behavior to be "perfect" :) and I think we should. We > just need to note this in any kind of migration docs. When I implemented this, libnm-core didn't exist yet. Since it is already done, I would not drop it. > > libnm: add function _nm_setting_get_setting_priority() > > Any reason for the swap of args in the return of > _nm_setting_type_is_base_type()? undone the swapping. > > core: add nm_utils_log_connection_diff > > struct _log_connection_setting_data > > We can just do "typedef struct { } LogConnectionSettingData" there to keep > style with the rest of NM. I know that's not your preferred style, but lets > try to keep it consistent... :) done. > _log_connection_get_property() - the comment "/* for NULL, we don't want to > return the string "NULL". */" doesn't make sense to me, because it looks like > the code does exactly that and returns "NULL". Was the comment switched > around? fixed. Repushed
> libnm-util: don't assert in nm_setting_get_secret_flags() and avoid assertion in agent_secrets_done_cb() + if (g_hash_table_size (setting_hash) <= 0) + continue; compares a guint value to <= 0. It's never going to be < 0 :) Also, for_each_secret() should always be called with a callback, otherwise it's pointless to call this function. So instead of doing "if (callback && callback ())" we should just g_return_if_fail (callback);. Rest looks fine.
merged as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ec84e4f00a9607ccea0139d728d24c2c2d714e6b