GNOME Bugzilla – Bug 767776
Non need options (arp_*) in bond mode 802.3ad
Last modified: 2016-07-06 13:48:16 UTC
OS: Linux 4.5.6-200.fc23.x86_64 NM: 1.0.12-2 plugins=ifcfg-rh,ibft If configuring a bond in mode 4 (802.3ad) the arp_[validate|interval] options are applied. They don't appear in the ifcfg-file but the system logs says: kernel: bond0: option arp_validate: mode dependency failed, not supported in mode 802.3ad(4) Every time the message appear, the network-connection is interrupted for a few seconds.
please show the output of `nmcli connection show $NAME` and attach the logfile with TRACE level enabled.
Created attachment 329932 [details] Logfile Output of "nmcli c show bond-bond0": connection.id: bond-bond0 connection.uuid: 6424823a-4f0c-4af6-8173-a464a61d37b2 connection.interface-name: bond0 connection.type: bond connection.autoconnect: yes connection.autoconnect-priority: 0 connection.timestamp: 0 connection.read-only: no connection.permissions: connection.zone: -- connection.master: -- connection.slave-type: -- connection.autoconnect-slaves: -1 (Vorgabe) connection.secondaries: connection.gateway-ping-timeout: 0 connection.metered: unbekannt ipv4.method: auto ipv4.dns: ipv4.dns-search: ipv4.addresses: ipv4.gateway: -- ipv4.routes: ipv4.route-metric: -1 ipv4.ignore-auto-routes: no ipv4.ignore-auto-dns: no ipv4.dhcp-client-id: -- ipv4.dhcp-send-hostname: yes ipv4.dhcp-hostname: -- ipv4.never-default: no ipv4.may-fail: yes ipv6.method: auto ipv6.dns: ipv6.dns-search: ipv6.addresses: ipv6.gateway: -- ipv6.routes: ipv6.route-metric: -1 ipv6.ignore-auto-routes: no ipv6.ignore-auto-dns: no ipv6.never-default: no ipv6.may-fail: yes ipv6.ip6-privacy: -1 (unbekannt) ipv6.dhcp-send-hostname: yes ipv6.dhcp-hostname: -- bond.options: mode=802.3ad GENERAL.NAME: bond-bond0 GENERAL.UUID: 6424823a-4f0c-4af6-8173-a464a61d37b2 GENERAL.GERÄTE: bond0 GENERAL.STATUS: wird aktiviert GENERAL.VORGABE: nein GENERAL.STANDARD6: nein GENERAL.VPN: nein GENERAL.ZONE: -- GENERAL.DBUS-PFAD: /org/freedesktop/NetworkManager/ActiveConnection/15 GENERAL.CON-PFAD: /org/freedesktop/NetworkManager/Settings/5 GENERAL.SPEC-OBJECT: / GENERAL.MASTER-PFAD: --
10:06:27 <info> Activation (bond0) Beginning DHCPv4 transaction (timeout in 45 seconds) ... 10:07:12 <warn> (bond0): DHCPv4 request timed out. ... 10:07:16 <info> Activation (bond0) Beginning DHCPv4 transaction (timeout in 45 seconds) ... 10:08:01 <warn> (bond0): DHCPv4 request timed out. The real reason why the connection is interrupted for few seconds is that the connection has ipv4.method=auto, but no DHCP server is available; thus the activation fails and is retried. I think you should consider using a different IPv4 method as 'disabled' or 'manual'. IPv6 autoconfiguration should be disabled too if there is no IPv6 router on bond0. Regarding the error messages in setting bond options, probably we should fix them, but they are harmless.
Sorry ... my fault. :-/
Created attachment 330850 [details] [review] [PATCH 1/2] bond: improve compatibility check of options and modes
Created attachment 330851 [details] [review] [PATCH 2/2] bond: fix default value for 'ad_actor_system' option
Patches should fix the errors.
Having the check of parameters against the modes is great. Patches lgtm.
(In reply to Beniamino Galvani from comment #5) > Created attachment 330850 [details] [review] [review] > [PATCH 1/2] bond: improve compatibility check of options and modes libnm-core already has those defines and strings too. Can we combine the code there? (as internal API is fine, nm-core-internal.h).
(In reply to Thomas Haller from comment #9) > (In reply to Beniamino Galvani from comment #5) > > Created attachment 330850 [details] [review] [review] [review] > > [PATCH 1/2] bond: improve compatibility check of options and modes > > libnm-core already has those defines and strings too. Can we combine the > code there? (as internal API is fine, nm-core-internal.h). Ok, pushed branch bg/bond-options-validate-bgo767776.
(In reply to Beniamino Galvani from comment #10) > Ok, pushed branch bg/bond-options-validate-bgo767776. >> bond: improve compatibility check of options and modes Could we get rid of NMDeviceBondPrivate.mode? Instead, the mode should be passed on set_bond_attr() as argument. I dislike here, that the mode only makes sense while setting the bond properties, it should not be cached in NMDeviceBondPrivate because they have different lifetimes. Just pass it on as argument. _ENTRY (AD_USER_PORT_KEY, ~(BIT (NM_BOND_MODE_8023AD))), If you grep for NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY, you don't find this occurrence. Usually I avoid "NM_SETTING_BOND_OPTION_ ## name" for that reason. >> bond: fix default value for 'ad_actor_system' option /* Any option that passes nm_setting_bond_validate_option() should also be found in defaults */ g_assert_not_reached (); + return NULL; Did you get a compiler warning about these? Strange. Maybe instead g_return_val_if_reached()? It is more graceful than assert and serves the purpose just as well. >> bond: use defines for sysfs attribute names I think, previously they were handled separately, to allow for the #define to differ from the kernel's sysctl name. But AFAIS, they always match, and it makes sense that we force them to be identical. + if (strcmp (option, NM_SETTING_BOND_OPTION_ARP_INTERVAL) && + strcmp (option, NM_SETTING_BOND_OPTION_DOWNDELAY) && NM_IN_STRSET()?
(In reply to Thomas Haller from comment #11) > >> bond: improve compatibility check of options and modes > > Could we get rid of NMDeviceBondPrivate.mode? Instead, the mode should be > passed on set_bond_attr() as argument. I dislike here, that the mode only > makes sense while setting the bond properties, it should not be cached in > NMDeviceBondPrivate because they have different lifetimes. Just pass it on > as argument. Ok. > If you grep for NM_SETTING_BOND_OPTION_AD_USER_PORT_KEY, you don't find this > occurrence. Usually I avoid "NM_SETTING_BOND_OPTION_ ## name" for that > reason. Changed. > >> bond: fix default value for 'ad_actor_system' option > > /* Any option that passes nm_setting_bond_validate_option() should also > be found in defaults */ > g_assert_not_reached (); > + return NULL; > > Did you get a compiler warning about these? Strange. No, but g_assert_not_reached() can be a no-op, so I think a value must be always returned. > Maybe instead g_return_val_if_reached()? It is more graceful than assert and > serves the purpose just as well. Yeah, g_return_val_if_reached() seems better as it always returns. I'll submit a separate patch to fix that globally. > >> bond: use defines for sysfs attribute names > > I think, previously they were handled separately, to allow for the #define > to differ from the kernel's sysctl name. But AFAIS, they always match, and > it makes sense that we force them to be identical. > > + if (strcmp (option, NM_SETTING_BOND_OPTION_ARP_INTERVAL) && > + strcmp (option, NM_SETTING_BOND_OPTION_DOWNDELAY) && > > NM_IN_STRSET()? Changed and repushed, thanks.
(In reply to Beniamino Galvani from comment #12) > > Did you get a compiler warning about these? Strange. > > No, but g_assert_not_reached() can be a no-op, so I think a value must be > always returned. Yes, if you compile with "#define G_DISABLE_ASSERT 1", then this statement is removed, and we get a compiler warning (I think a missing return statement in C is ~only~ a warning. If you compile with g_return/g_assert disabled, I suspect you get *a lot* of compiler warnings, thus in that mode you just cannot compile --with-more-warnings=error. It's already hard enough to ensure regular builds without compiler warnings in common environments. Also getting that right for disabling assertions is not feasible IMO. That's why I actually prefer g_return_if_reached(), because that code *always* has a return statement there. Sometimes I do therefore: if (!cond) g_return_val_if_reached (FALSE); instead of g_return_val_if_fail (cond, FALSE); But note, that the code really should be reached, either in debug nor release builds. So it actually shouldn't matter. Pushed one fixup. lgtm now.
(In reply to Thomas Haller from comment #13) > If you compile with g_return/g_assert disabled, I suspect you get *a lot* of > compiler warnings, thus in that mode you just cannot compile > --with-more-warnings=error. It's already hard enough to ensure regular > builds without compiler warnings in common environments. Also getting that > right for disabling assertions is not feasible IMO. > > That's why I actually prefer g_return_if_reached(), because that code > *always* has a return statement there. Sometimes I do therefore: > > if (!cond) > g_return_val_if_reached (FALSE); > > instead of > > g_return_val_if_fail (cond, FALSE); > > But note, that the code really should be reached, either in debug nor > release builds. So it actually shouldn't matter. Agree, let's keep it as is. > Pushed one fixup. Squashed and merged, thanks. https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9a6412e3aaf92d6b4b956a0be43dae9feffc7dff