Bug 1887523

`nm_setting_bond_add_option()` does not perform such kind of validations, nor should it.

For one, that is because you always could also set the GObject property (NM_SETTING_BOND_OPTIONS), and when setting a GObject property, there is no place to validate the input (as `g_object_set()` has no return value to indicate failure).

In general, in libnm it is not the setters that perform validation. Validation should be performed by nm_connection_verify(). That is for example also because often the validity of a setting depends on multiple properties (like `packets_per_slave` only works with certain bond modes). Validating values in the setter would mean that you need to set the options in a certain order. Say you start with a setting "mode=balance-rr, packets_per_slave=1" and you want to get to "mode=802.3ad, ad_actor_sys_prio=5". It means, you either first set "mode=802.3ad" (but then "packets_per_slave=1" would be wrong/inconsistent), or you first set "ad_actor_sys_prio=5" (but then the mode would be wrong).


Actually, add-option does perform some basic form of validation. For example, "packets_per_slave" is validated to be a string. I think that is an actual bug, because it means you can set options via NM_SETTING_BOND_OPTIONS to anything, but there is no corresponding C API to achieve the same. That is a problem, because keyfile reader uses nm_setting_bond_add_option(), and -- in case of an invalid value -- it silently ignores certain values.


Actually, nm_connection_verify() for bond options has a different problem: historically the validation was not strict and accept some settings that would not be valid. This was not fixed so far, because making validation more strict is a breaking change in behavior. Maybe, nm_connection_verify() should be tightend up for bond settings, but it's problematic to do so.


What also should be done is `nm_setting_bond_add_option()` consistently does *not* perform any of the basic validation. Because the basic validation should only happen during nm_connection_verify(), and keyfile reader would better handle reading invalid values -- of course, that would also be a breaking change in behavior *sigh*.


The way bond options are handled by libnm/NMSettingBond is rather ugly, and fixing it would break existing behavior.

Gris does the '`packets_per_slave` option get rejected when bond-mode is not 'balance-rr' when using nm_connection_verify()?

The `NM.RemoteConnection.verify()` return True when holding `NM.SettingBond` with `packets_per_slave: 1` and `mode: active-backup`.

Nmstate has verification stage which will warn user about invalid bond options, so I don't expect NM fix it any time soon. Just want to let you know the issue exists.

As the API user of libnm, the document said `add_option(): True if the option was valid and was added to the internal option list, False if it was not.`. And the document also have `validate_option()` which still return `True` in this case.
Please update the document of API also.

> The `NM.RemoteConnection.verify()` return True when holding `NM.SettingBond` with `packets_per_slave: 1` and `mode: active-backup`.

Yes, that's because 

> > Actually, nm_connection_verify() for bond options has a different problem: historically the validation was not strict and accept some settings that would not be valid. This was not fixed so far, because making validation more strict is a breaking change in behavior. Maybe, nm_connection_verify() should be tightend up for bond settings, but it's problematic to do so.




https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/653

^^ this disables all validation of nm_setting_bond_add_option() and adjust documentation. It does not add stricter validation for nm_connection_verify(). Maybe that should be done in the future, but it's problematic.

Gris are you happy with this solution? I know it's not ideal but we might break a lot of (incorrect) connection profiles if we make the verify() stricter.

Unfortunately options which are not compatible with the bond mode get stripped from the connection profile in NM.

I am OK with NM remove all check on bond options and let others do the work.

Note that https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/653 only removes the checks when adding an option through 'nm_setting_bond_add_option()', 'nm_connection_verify()' will still do all the previous checks on the bond connection options.

I am OK on `nm_setting_bond_add_option` removing the validation work.

NetworkManager-1.30.0-0.2.el8.x86_64.rpm passed bond test of nmstate CI.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (Moderate: NetworkManager and libnma security, bug fix, and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2021:1574