GNOME Bugzilla – Bug 771579
[review] Activation completes immediately when there is a disabled/ignore method
Last modified: 2016-09-22 12:19:43 UTC
From commit message: If ipx_state is set to IP_DONE when the method is disabled/ignore, the activation will proceed immediately without waiting the other method to complete. Instead, introduce a new IP_DISABLED state so that we can distinguish whether the configuration really took place and we can change the device state accordingly.
Created attachment 335754 [details] [review] [PATCH] device: don't set IP_DONE for ipvx.method=disabled/ignore
Review of attachment 335754 [details] [review]: ::: src/devices/nm-device.c @@ +5456,3 @@ } else if (strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_DISABLED) == 0) { apply_mtu_from_config (self); + ret = NM_ACT_STAGE_RETURN_IP_DONE; this returned NM_ACT_STAGE_RETURN_SUCCESS on purpose. The reason is that configuring method=disabled is the same as having no IPv4 configuration. Before, NM_ACT_STAGE_RETURN_IP_DONE means to directly switch to IP_DONE state, without clearing IP configuration. That is what's done for "ipv6.method=ignore" -- which is not the same as "ipv4.method=disabled". it seems to me that the new state "IP_DISABLED" is correct to express ipv6.method=ignored, but it is wrong to express "ipv4.method=disabled". How about not introducing a new IP_DISABLED state. Isn't this only a bug in check_ip_done() to treat IP_DONE with "ipv4.method=disabled" wrongly? It should see that IPv4 is disabled&DONE, then it should still wait for ipv6. @@ +6865,3 @@ g_object_unref (ip4_config); } else if (ret == NM_ACT_STAGE_RETURN_IP_DONE) { + _set_ip_state (self, AF_INET, IP_DISABLED); NM_ACT_STAGE_RETURN_IP_DONE seems now to be the wrong name now.
Created attachment 335903 [details] [review] [PATCH v2] device: don't progress state immediately when ipvx.method=disabled/ignore (In reply to Thomas Haller from comment #2) > Review of attachment 335754 [details] [review] [review]: > > ::: src/devices/nm-device.c > @@ +5456,3 @@ > } else if (strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_DISABLED) == 0) { > apply_mtu_from_config (self); > + ret = NM_ACT_STAGE_RETURN_IP_DONE; > > this returned NM_ACT_STAGE_RETURN_SUCCESS on purpose. The reason is that > configuring method=disabled is the same as having no IPv4 configuration. > > Before, NM_ACT_STAGE_RETURN_IP_DONE means to directly switch to IP_DONE > state, without clearing IP configuration. That is what's done for > "ipv6.method=ignore" -- which is not the same as "ipv4.method=disabled". Ok, fixed. > it seems to me that the new state "IP_DISABLED" is correct to express > ipv6.method=ignored, but it is wrong to express "ipv4.method=disabled". > > > How about not introducing a new IP_DISABLED state. Isn't this only a bug in > check_ip_done() to treat IP_DONE with "ipv4.method=disabled" wrongly? It > should see that IPv4 is disabled&DONE, then it should still wait for ipv6. How about patch v2? The downside is that we have to look up the setting and properties every time, but I think it's easier to understand now.
Review of attachment 335903 [details] [review]: The change to check_ip_done() looks good. I think it's easier to understand then having adding a new state with a IP_DONE_WAIT semantics. ::: src/devices/nm-device.c @@ +4176,3 @@ + /* Don't fail the connection if one non-disabled IPv4/IPv6 succeeded */ + if ( (priv->ip4_state != IP_FAIL && !ip4_disabled) + || (priv->ip6_state != IP_FAIL && !ip6_ignore)) why do you also modify check_ip_failed()? It seems to me, we only need a special handling in check_ip_done() for IP_DONE&&ip4_disabled state to implement this IP_DONE_WAIT pseudo-state. With IP_FAIL state, it seems that a fail always means fail. Regardles of ip4_disabled/ip6_ignore. Actually, with the recent change, I don't even think that the state can be IP_FAIL with a disabled/ignored IP method.
(In reply to Thomas Haller from comment #4) > Review of attachment 335903 [details] [review] [review]: > > The change to check_ip_done() looks good. I think it's easier to understand > then having adding a new state with a IP_DONE_WAIT semantics. > > ::: src/devices/nm-device.c > @@ +4176,3 @@ > + /* Don't fail the connection if one non-disabled IPv4/IPv6 succeeded */ > + if ( (priv->ip4_state != IP_FAIL && !ip4_disabled) > + || (priv->ip6_state != IP_FAIL && !ip6_ignore)) > > why do you also modify check_ip_failed()? Because we must ensure that when the connection has IPv4=disabled,IPv6=auto and IPv6 fails the activation will fail, even if IPv6.may-fail=yes. We have this assumption that at least one method (!= disabled/ignore) must succeed for the connection to be activated. The case is which both are disabled is special, though. The check is needed to consider any disabled method as failed and preserve the old behavior. > It seems to me, we only need a special handling in check_ip_done() for > IP_DONE&&ip4_disabled state to implement this IP_DONE_WAIT pseudo-state. > With IP_FAIL state, it seems that a fail always means fail. Regardles of > ip4_disabled/ip6_ignore. Actually, with the recent change, I don't even > think that the state can be IP_FAIL with a disabled/ignored IP method.
ok, what you describe here makes sense... ignoring the fact that I find this behavior complicated. But I don't see where you check that: + /* Don't fail the connection if one non-disabled IPv4/IPv6 succeeded */ + if ( (priv->ip4_state != IP_FAIL && !ip4_disabled) + || (priv->ip6_state != IP_FAIL && !ip6_ignore)) return; I think it should be: if (ip4_disabled && ip6_ignore) { /*add a comment with what you just said */ return; } if ( !(priv->ip4_state == IP_FAIL && ip6_ignored)) && !(priv->ip6_state == IP_FAIL && ip4_disabled)) { /* add a comment with what you just said. */ return; } below there is: } else if ( may_fail && get_ip_config_may_fail (self, AF_INET) && get_ip_config_may_fail (self, AF_INET6)) { /* Couldn't start either IPv6 and IPv4 autoconfiguration, I cannot wrap my head around why the check for may-fail is not coupled with a check for ip_state. Before your patch already, with ip4_state==IP_FAIL,ip4.may-fail=no,ip6_state=IP_WAIT we would switch to secondaries... that seems wrong. Why is there even a ip_check_fail() and ip_check_done() function instead of an ip_check() state? Maybe those two should be combined in one function?
(In reply to Thomas Haller from comment #6) > ok, what you describe here makes sense... ignoring the fact that I find this > behavior complicated. > > But I don't see where you check that: > > + /* Don't fail the connection if one non-disabled IPv4/IPv6 succeeded */ > + if ( (priv->ip4_state != IP_FAIL && !ip4_disabled) > + || (priv->ip6_state != IP_FAIL && !ip6_ignore)) > return; > > I think it should be: > > if (ip4_disabled && ip6_ignore) { > /*add a comment with what you just said */ > return; > } Ok. > if ( !(priv->ip4_state == IP_FAIL && ip6_ignored)) > && !(priv->ip6_state == IP_FAIL && ip4_disabled)) { > /* add a comment with what you just said. */ > return; > } I don't think this is correct. The patch contains: /* Don't fail the connection if one non-disabled IPv4/IPv6 succeeded */ if ( (priv->ip4_state != IP_FAIL && !ip4_disabled) || (priv->ip6_state != IP_FAIL && !ip6_ignore)) return; which means that if IPv4 succeeded (or is still pending) AND it's not disabled: if (priv->ip4_state != IP_FAIL && !ip4_disabled) we shouldn't fail (yet) the connection, hence we return. The same for IPv6. Your version returns only when one IP state is IP_FAIL and also disabled. > below there is: > > } else if ( may_fail > && get_ip_config_may_fail (self, AF_INET) > && get_ip_config_may_fail (self, AF_INET6)) { > /* Couldn't start either IPv6 and IPv4 autoconfiguration, > > I cannot wrap my head around why the check for may-fail is not coupled with > a check for ip_state. > Before your patch already, with > ip4_state==IP_FAIL,ip4.may-fail=no,ip6_state=IP_WAIT we would switch to > secondaries... that seems wrong. Before my patch the function would return immediately in presence of non-failed states (where disabled method were considered as failed): if ( priv->ip4_state != IP_FAIL || priv->ip6_state != IP_FAIL) return; and so that code would not be reached. This shouldn't be different after my patch. > Why is there even a ip_check_fail() and ip_check_done() function instead of > an ip_check() state? Maybe those two should be combined in one function? I'll try to unify them.
(1): > Because we must ensure that when the connection has IPv4=disabled,IPv6=auto and > IPv6 fails the activation will fail, even if IPv6.may-fail=yes. (2): > which means that if IPv4 succeeded (or is still pending) AND it's > not disabled: Yes, (2) describes the condition in the patch. But that isn't the same as (1). I think to implement (1), you must compare the IPv4 state with IPv6-ignored (and vice-versa). > Your version returns only when one IP state is IP_FAIL and also disabled. I compared ip4_state == IP_FAIL with IPv6-disabled. From (1). Ok, I lost track. This is quite complicated. Let's see how they look after being unified.
Created attachment 336050 [details] [review] [PATCH v3] device: rework state transition after IP configuration How about this?
Review of attachment 336050 [details] [review]: Looks good to me. I think it's easier to understand then before. ::: src/devices/nm-device.c @@ +4179,3 @@ + + if ( ip4_disabled && priv->ip4_state == IP_DONE + && ip6_ignore && priv->ip6_state == IP_DONE) { doesn't need to ip4_disabled/ip6_ignore check. If both are DONE, we anyway can proceed.
(In reply to Thomas Haller from comment #10) > + if ( ip4_disabled && priv->ip4_state == IP_DONE > + && ip6_ignore && priv->ip6_state == IP_DONE) { > > doesn't need to ip4_disabled/ip6_ignore check. If both are DONE, we anyway > can proceed. Right, good point.
Applying Thomas advice in check_ip_state (), seems to me that these two last block of code can be removed: + /* If a method is still pending but required, wait */ + if (priv->ip4_state != IP_DONE && !get_ip_config_may_fail (self, AF_INET)) + return; + if (priv->ip6_state != IP_DONE && !get_ip_config_may_fail (self, AF_INET6)) + return; + + /* If at least a method has completed, proceed with activation */ + if ( (priv->ip4_state == IP_DONE && !ip4_disabled) + || (priv->ip6_state == IP_DONE && !ip6_ignore)) { + nm_device_state_changed (self, NM_DEVICE_STATE_IP_CHECK, NM_DEVICE_STATE_REASON_NONE); + return; + } Patch LGTM
(In reply to Francesco Giudici from comment #12) > Applying Thomas advice in check_ip_state (), seems to me that these two last > block of code can be removed: They are actually needed because the condition from comment 11 only returns when both states are IP_DONE, while the condition below has a ||. > Patch LGTM Merged to master, thanks. https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=f1165cc29075cd12d5fff5226e5eff6735187885