Skip to content
GitLab

[th/connection-permissions-cleanup] libnm: cleanup handling of "connection.permissions" and improve validation

Closed Thomas Haller requested to merge th/connection-permissions-cleanup into master

Previously, both nm_setting_connection_add_permission() and the GObject property setter would merely assert that the provided values are valid (and otherwise don't do anything). That is bad for handling errors.

For example, we use the property setter to initialize the setting from keyfile and GVariant (D-Bus). That means, if a user provides an invalid permissions value, we would emit a g_critical() assertion failure, but otherwise ignore the permissions. What we instead need to do is to accept the value, and afterwards fail verification. That way, a proper error message can be generated.

  $ mcli connection add type ethernet autoconnect no ifname bogus con-name x connection.permissions 'bogus:'

  (process:429514): libnm-CRITICAL **: 12:12:00.359: permission_new: assertion 'strchr (uname, ':') == NULL' failed

  (process:429514): libnm-CRITICAL **: 12:12:00.359: nm_setting_connection_add_permission: assertion 'p != NULL' failed
  Connection 'x' (2802d117-f84e-44d9-925b-bfe26fd85da1) successfully added.
  $ $  nmcli -f connection.permissions connection show x
  connection.permissions:                 --

While at it, also don't track the permissions in a GSList. Tracking one permission in a GSList requires 3 allocations (one for the user string, one for the Permission struct, and one for the GSList struct). Instead, use a GArray. That is still not great, because GArray cannot be embedded inside NMSettingConnectionPermission, so tracking one permission requires also 3 allocations (which is really a fault of GArray). So, GArray is not better in the most common case where there is only one permissions. But even in the worst case (only one entry), GArray is not worse than GSList.

Also change the API of nm_setting_connection_add_permission(). Previously, the function would assert that the arguments are in a certain form (strcmp (ptype, "user") == 0), but still document the such behaviors like regular operation ("[returns] %FALSE if @ptype or @pitem was invalid"). Don't assert against the return values. Also, if you first set the user to "fo:o", then nm_setting_connection_add_permission() would accept it -- only at a later phase, the property setter would assert against such values. Also, the function would return %FALSE both if the input value was invalid (an error) and if the value already existed. I think the function should not treat a duplicate entry like a badly formatted input. Now the function does much less asserting of the arguments, but will return %FALSE if the values are invalid. And it will silently ignore duplicate entries.

Merge request reports