Bug 1171009 - mode=<numeric> in BONDING_OPTS is not supported
Summary: mode=<numeric> in BONDING_OPTS is not supported
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager
Version: 7.0
Hardware: All
OS: Linux
unspecified
unspecified
Target Milestone: rc
: ---
Assignee: Jirka Klimes
QA Contact: Desktop QE
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-12-05 09:30 UTC by Takayuki Nagata
Modified: 2019-04-16 14:26 UTC (History)
12 users (show)

Fixed In Version: NetworkManager-1.0.0-9.git20150121.b4ea599c.el7
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-03-05 13:54:06 UTC
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Knowledge Base (Solution) 1282993 None None None Never
Red Hat Product Errata RHBA-2015:0311 normal SHIPPED_LIVE NetworkManager bug fix and enhancement update 2015-03-05 17:35:10 UTC

Comment 3 Jirka Klimes 2014-12-05 09:54:23 UTC
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).

Comment 4 Jirka Klimes 2014-12-17 15:29:40 UTC
Fixes are in upstream NetworkManager branch jk/numeric-bond-modes.

Comment 5 Thomas Haller 2014-12-17 16:11:45 UTC
>> 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.

Comment 6 Lubomir Rintel 2015-01-07 10:09:10 UTC
> 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

Comment 7 Dan Winship 2015-01-07 13:42:42 UTC
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.

Comment 8 Jirka Klimes 2015-01-12 14:03:20 UTC
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).

Comment 9 Dan Winship 2015-01-12 14:48:53 UTC
(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.

Comment 10 Thomas Haller 2015-01-12 22:01:56 UTC
(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.

Comment 11 Jirka Klimes 2015-01-13 09:03:30 UTC
The code has been merged to master. See upstream bug https://bugzilla.gnome.org/show_bug.cgi?id=704666#c2.

Comment 19 errata-xmlrpc 2015-03-05 13:54:06 UTC
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


Note You need to log in before you can comment on or make changes to this bug.