Bug 1171009

Initscripts (ifup-eth) just echoes the bonding mode to /sys/class/net/<interface>/bonding/mode. And the sysfs accepts both strings and numeric values.
We will need to update ifcfg-rh plugin to accept numeric values too.

This bug is related to bug 1133544 (accepting numeric bond modes on D-Bus).

Fixes are in upstream NetworkManager branch jk/numeric-bond-modes.

>> utils: add functions for converting string <-> numeric bonding modes

nm_utils_bond_mode_string_to_int() et al. should be implemented based on a utility function...

for example something like __str2type() in libnl:
https://github.com/thom311/libnl/blob/b56f88c9b3c22466fabeb1d751666bc4f79d347c/lib/utils.c#L1019

Note that other bond-parameters also come as name-number pair, so there is potential for reuse.


Some trailing white spaces.



Looking at the later patches, these new functions are never used outside NM/libnm-core. Let's not make them public API -- at this point. We can do it later anytime...



>> ifcfg-rh: accept numeric bonding mode values in BONDING_OPTS (rh #1171009)

why do we need this change? Can we not just read the string value (be it numeric or descriptive) and nm_setting_bond_add_option() ?

nm_setting_bond_add_option() should accept both. reader() should not pre-fix it.

The tests are useful.

>> bond: normalize bond mode to string notation

same here. Now that NMSettingBond:verify() accept numerics, I don't think we should event try to normalize the settings. Numerics are just fine.

But if we really want to, then this commit obsoletes the change in "accept numeric bonding mode values in BONDING_OPTS" because after reading a connection we normalize it already.



I would reorder the commits:
 1 utils: add functions for converting string <-> numeric bonding modes
 2 libnm-core: accept numeric bond modes (bgo #704666) (rh #1133544)
 3 bond: normalize bond mode to string notation
 4 ifcfg-rh: accept numeric bonding mode values in BONDING_OPTS (rh #1171009)
with dropping 4 (except the tests part!!), possibly dropping 3.

> utils: add functions for converting string <-> numeric bonding modes

> + * Returns: bonding mode string, or NULL on error
> +*/ 
     ^ a trailing space

Also, couldn't we add the list of valid options to libnm so that clients can reuse them?

Otherwise LGTM

Copying from bug 1133544 so that all the reviews are in one place:

It looks like you never use nm_utils_bond_mode_string_to_int() and nm_utils_bond_mode_int_to_string() independently of each other; you could just replace them with a single

  const char *nm_utils_bond_mode_normalize (const char *mode);


"ifcfg-rh: accept numeric bonding mode values in BONDING_OPTS" should not actually be needed after the normalization patch. ifcfg-rh can just set the option as the numeric string, and it will get normalized to the string value later on.

Rebased to master with these changes:
* fix whitespaces
* reorganized the commits per Thomas suggestion
* dropped int -> string change from the ifcfg-rh reader

Other notes:
- I was about to use nm_utils_bond_mode_string_to_int(), int_to_string() in nmcli as well at one point, but dropped later. But I still see a point having both functions in utils as they are handy for g-object introspection scripts or other clients.
- Even if numeric is acceptable in the mode option, I prefer to have it just in one format (string).

(In reply to Jirka Klimes from comment #8)
> - I was about to use nm_utils_bond_mode_string_to_int(), int_to_string() in
> nmcli as well at one point, but dropped later. But I still see a point
> having both functions in utils as they are handy for g-object introspection
> scripts or other clients.

OK, well, in that case, I think think it's weird that string_to_int() accepts integers-as-strings. Yes, it's more *convenient*, but it's weird.

But your call. The branch is OK as is.

(I still would not normalize the bond mode)

Maybe nm_utils_bond_mode_normalize() as danw suggests: https://bugzilla.redhat.com/show_bug.cgi?id=1133544#c5 


Other then that, it looks very good (as is). Your call.

The code has been merged to master. See upstream bug https://bugzilla.gnome.org/show_bug.cgi?id=704666#c2.

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, 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://rhn.redhat.com/errata/RHBA-2015-0311.html