branch: th/route-fixes-bgo741871

>    ifcfg-rh: support ipvx.route-metric property as IPVX_ROUTE_METRIC

>+	 * variable: IPV4_ROUTE_METRIC(+)
>+	 * default: -1
>+	 * description: IPV4_ROUTE_METRIC is the default IPv4 metric for routes on this connection.

should explain what happens if this is "-1"

>+	              NM_SETTING_IP_CONFIG_ROUTE_METRIC, (gint64) 204,

AARGH! I hadn't thought of that (the fact that you'd need a cast when using varargs APIs) when we added the property. :-/. That's going to lead to bugs. I'm now pre-emptively registering a strong objection to any future 64-bit properties.



>    device: log sysctl values in device constructor for debugging

>+	if (   nm_logging_enabled (LOGL_DEBUG, LOGD_PLATFORM)

I realize you're just taking advantage of the existing platform debug logging here, but it seems wrong to have this behavior depend on PLATFORM:DEBUG rather than DEVICE:DEBUG. (Also, nm-device.c shouldn't be making assumptions about what the platform will and won't log. What if we changed the sysctl_get logging to TRACE at some point in the future?)

>+	    && priv->iface
>+	    && nm_utils_iface_valid_name (priv->iface)) {

I think you want "!device_has_capability (self, NM_DEVICE_CAP_IS_NON_KERNEL)" here, not nm_utils_iface_valid_name(). (eg, "ttyUSB0" is a valid interface name, but it's not an interface)



>    platform: workaround deletion of IPv4 route with metric 0

I don't think this will work. If you've got:

192.168.1.0/24  dev wlp3s0  src 192.168.1.11  metric 600
192.168.1.0/24  dev wlp3s0  src 192.168.1.11  metric 0

then when you ask the kernel to delete the metric 0 route, it will delete the metric 600 route instead, because it sees that one first when iterating through the list, and as far as it's concerned, that route matches the deletion request.

(In reply to comment #2)
> >    ifcfg-rh: support ipvx.route-metric property as IPVX_ROUTE_METRIC
> 
> >+	 * variable: IPV4_ROUTE_METRIC(+)
> >+	 * default: -1
> >+	 * description: IPV4_ROUTE_METRIC is the default IPv4 metric for routes on this connection.

done


> should explain what happens if this is "-1"
> 
> >+	              NM_SETTING_IP_CONFIG_ROUTE_METRIC, (gint64) 204,
> 
> AARGH! I hadn't thought of that (the fact that you'd need a cast when using
> varargs APIs) when we added the property. :-/. That's going to lead to bugs.
> I'm now pre-emptively registering a strong objection to any future 64-bit
> properties.

The alternative would have been to add two properties like:

gboolean NM_SETTING_IP_CONFIG_USE_ROUTE_METRIC
guint32  NM_SETTING_IP_CONFIG_ROUTE_METRIC

I think the redundancy is worse then having a gint64 type.


> >    device: log sysctl values in device constructor for debugging
> 
> >+	if (   nm_logging_enabled (LOGL_DEBUG, LOGD_PLATFORM)
> 
> I realize you're just taking advantage of the existing platform debug logging
> here, but it seems wrong to have this behavior depend on PLATFORM:DEBUG rather
> than DEVICE:DEBUG. (Also, nm-device.c shouldn't be making assumptions about
> what the platform will and won't log. What if we changed the sysctl_get logging
> to TRACE at some point in the future?)

The logging of the sysvalues in platform is quite enhanced. For example it prints the value on previous read if it differs. Duplicating that would be overkill.
Also, NMDevice:constructed() could not call nm_platform_sysctl_get() if it would log the value itself (because then we have both LOGD_PLATFORM and LOGD_DEVICE lines).
And having a "silent" argument in nm_platform_sysctl_get() seems ugly too.

I see your point, but considering the options, I think this is best.

Vote?



> >+	    && priv->iface
> >+	    && nm_utils_iface_valid_name (priv->iface)) {
> 
> I think you want "!device_has_capability (self, NM_DEVICE_CAP_IS_NON_KERNEL)"
> here, not nm_utils_iface_valid_name(). (eg, "ttyUSB0" is a valid interface
> name, but it's not an interface)

done


> >    platform: workaround deletion of IPv4 route with metric 0
> 
> I don't think this will work. If you've got:
> 
> 192.168.1.0/24  dev wlp3s0  src 192.168.1.11  metric 600
> 192.168.1.0/24  dev wlp3s0  src 192.168.1.11  metric 0
> 
> then when you ask the kernel to delete the metric 0 route, it will delete the
> metric 600 route instead, because it sees that one first when iterating through
> the list, and as far as it's concerned, that route matches the deletion
> request.

I don't think that is the case. Why do you think that? It would mean, you cannot delete deterministically the metric0 route, without possibly killing the metric600 one. Would be quite a kernel bug.
When I try with iproute2, it correctly deletes the metric0 route.

But if you have only the metric600 route, and you try to delete metric0, it will delete metric600.



Rebased and repushed.

(In reply to comment #3)
> (In reply to comment #2)
> > I don't think this will work. If you've got:
> > 
> > 192.168.1.0/24  dev wlp3s0  src 192.168.1.11  metric 600
> > 192.168.1.0/24  dev wlp3s0  src 192.168.1.11  metric 0
> > 
> > then when you ask the kernel to delete the metric 0 route, it will delete the
> > metric 600 route instead, because it sees that one first when iterating through
> > the list, and as far as it's concerned, that route matches the deletion
> > request.
> 
> I don't think that is the case. Why do you think that?

Because that's how the kernel route deletion code works; it walks through the routing table and deletes the first route that matches the request, and "metric 0" matches any metric.

However, after some testing, it looks like this isn't actually a problem in practice, because you can't actually have the routing table example given above; routes get sorted by metric, so the metric 0 route will always come before the metric 600 route, regardless of which order you added them in, so your code will work.

> device: log sysctl values in device constructor for debugging

why not use gs_free here?

Also, WRT to "vote?" I'm also a bit uneasy about the dependency here.  I'd actually rather just double-log in both platform and device.

---

WRT guint64 properties, danw has a point and I'm 100% certain it'll cause bugs in the future.  We've already had exactly these bugs with Timestamp.  But I'm not sure there's much we can do about it, other than remember really really hard what values are 64-bit and cast them :(  But I agree with danw, we should review/discuss new 64-bit properties with extra scrutiny.

Merged patches to master except "device: log sysctl values in device constructor for debugging"

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=1bb1bfe6e7c8206ba5e157b15b1a4db81b24fd80

Created attachment 294270 [details] [review]
[patch] device: log sysctl values in device constructor for debugging

Reworked the remaining patch, dropping the macro.
This is the only remaining part.

What means "double-log" exactly?

Note that repeated calls to nm_platform_sysctl_get() do not log a line unless the value changed. That is pretty neat, because it removes redundant logging lines.

After merging the branch, I found an issue.
Fix it, and add a test function for deleting route with metric0.

New patches pushed to th/route-fixes-bgo741871-v2

>    platform/tests: add test for deleting IPv4 route with metric 0

>+	/*SignalData *route_changed = add_signal (NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, NM_PLATFORM_SIGNAL_CHANGED, ip4_route_callback);*/

>+	/*free_signal (route_changed);*/

#ifdef is better than commenting. Although I would just put the fix commit before the test commit.


(In reply to comment #7)
> What means "double-log" exactly?

he meant to log them at both DEVICE:DEBUG and PLATFORM:DEBUG

merged relevant parts as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=d9f143df8bc6a050653519e242565131cf7c5905

(In reply to comment #9)
> >    platform/tests: add test for deleting IPv4 route with metric 0
> 
> >+	/*SignalData *route_changed = add_signal (NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, NM_PLATFORM_SIGNAL_CHANGED, ip4_route_callback);*/
> 
> >+	/*free_signal (route_changed);*/
> 
> #ifdef is better than commenting. Although I would just put the fix commit
> before the test commit.

the idea was to add first the test (without checking for the CHANGED signal) and then fix the issue. So that it is clearer what the next commit is supposed to fix.

> (In reply to comment #7)
> > What means "double-log" exactly?
> 
> he meant to log them at both DEVICE:DEBUG and PLATFORM:DEBUG

I dropped the patch. It's still in attachment 294270 [details] [review] for later reference