After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 767776 - Non need options (arp_*) in bond mode 802.3ad
Non need options (arp_*) in bond mode 802.3ad
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
1.0.x
Other Linux
: Normal critical
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks: nm-review
 
 
Reported: 2016-06-17 06:32 UTC by Benjamin
Modified: 2016-07-06 13:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Logfile (487.18 KB, text/plain)
2016-06-17 08:12 UTC, Benjamin
  Details
[PATCH 1/2] bond: improve compatibility check of options and modes (11.52 KB, patch)
2016-07-04 15:09 UTC, Beniamino Galvani
none Details | Review
[PATCH 2/2] bond: fix default value for 'ad_actor_system' option (6.93 KB, patch)
2016-07-04 15:10 UTC, Beniamino Galvani
none Details | Review

Description Benjamin 2016-06-17 06:32:59 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.
Comment 1 Thomas Haller 2016-06-17 07:41:07 UTC
please show the output of `nmcli connection show $NAME` and attach the logfile with TRACE level enabled.
Comment 2 Benjamin 2016-06-17 08:12:39 UTC
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:                    --
Comment 3 Beniamino Galvani 2016-06-21 12:08:45 UTC
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.
Comment 4 Benjamin 2016-06-22 06:45:46 UTC
Sorry ... my fault. :-/
Comment 5 Beniamino Galvani 2016-07-04 15:09:50 UTC
Created attachment 330850 [details] [review]
[PATCH 1/2] bond: improve compatibility check of options and modes
Comment 6 Beniamino Galvani 2016-07-04 15:10:47 UTC
Created attachment 330851 [details] [review]
[PATCH 2/2] bond: fix default value for 'ad_actor_system' option
Comment 7 Beniamino Galvani 2016-07-04 15:11:17 UTC
Patches should fix the errors.
Comment 8 Francesco Giudici 2016-07-05 13:01:19 UTC
Having the check of parameters against the modes is great. 
Patches lgtm.
Comment 9 Thomas Haller 2016-07-05 13:24:00 UTC
(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).
Comment 10 Beniamino Galvani 2016-07-06 07:39:20 UTC
(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.
Comment 11 Thomas Haller 2016-07-06 08:57:42 UTC
(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()?
Comment 12 Beniamino Galvani 2016-07-06 13:00:31 UTC
(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.
Comment 13 Thomas Haller 2016-07-06 13:13:55 UTC
(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.
Comment 14 Beniamino Galvani 2016-07-06 13:48:16 UTC
(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