Please review th/bg719851_keyfile_route_addr_fix



Not that this changes how routes are written, after the patch it would write:

[ip4]
route1=3.4.5.6/26,,1 (a)
[ip6]
route1=6:7:8:9:0:1:2:3/126,,1  (b)


(b) is no problem, because without this patch, current NM would actually write
"route1=6:7:8:9:0:1:2:3/126,1" which is wrong anyway -- although with this patch, such a wrong route could be re-read again.


(a) might be a problem, because if a new NM writes such a route, and older version of NM could not re-read it again. Older NM would write and expect "route1=3.4.5.6/26,0.0.0.0,1" instead.
I think, this is acceptable, because it just means, that newer NM writes a configuration, that older NM does not understand (in a rather unusual case).

(In reply to comment #1)

> (b) is no problem, because without this patch, current NM would actually write
> "route1=6:7:8:9:0:1:2:3/126,1" which is wrong anyway -- although with this
> patch, such a wrong route could be re-read again.

Actually, if NM would not omit the :: gateway, old NM could actually read
route1=6:7:8:9:0:1:2:3/126,::,1

Don't know, if we care about this(?) Should we always write the gateway, even if it is 0.0.0.0/:: ?

(In reply to comment #1)
> I think, this is acceptable, because it just means, that newer NM writes a
> configuration, that older NM does not understand (in a rather unusual case).

Well, I had to keep switching back to NM 0.9.8 to test things while doing those releases, and it would have been annoying if some of my connections hadn't worked.

But it sounds from your last comment like you've found a fully-compatible way to write things anyway? If so, go with that.

reworked, rebased to master and repushed.

Now writer will/should always write something, that also old versions can read too.

> keyfile: fix reader and writer for writing routes

in reader.c::build_ip6_address_or_route():

+ if (!metric_str && get_one_int (gateway_str, G_MAXUINT32, NULL, &metric)) {
+     addr = in6addr_any;
+     metric_str = NULL;

can remove the metric_str = NULL there since it was already checked for NULL just above.

Also, it looks like the branch adds then removes the reader testcases for "route3=6:7:8:9:0:1:2:3/126,::,1"; should we add that back in as a valid testcase?  If somebody hand-writes the keyfile, I can see them using that notation and that should probably be valid.

writer.c::write_ip4_values():

I don't think we need the (unsigned long) cast for plen at all?  The prefix will never be > 128, and the 'plen' variable is a 'guint32', so it shouldn't need a long conversion at all.  I think we can go with just "%u" here and no cast.  Same thing for 'metric' too.

Also, instead of "allow_omitting_specified_gateway", perhaps it should be "force_write_gateway"?  That would change the if() in ip6_array_to_addr_prefix() to a clearer:

 if (force_write_gateway || !IN6_IS_ADDR_UNSPECIFIED())

The parameter's intention is to force writing the gateway or to leave it up to IN6_IS_ADDR_UNSPECIFIED(), and I think 'force_write_gateway' is a clearer name here.  At least in my head, when I'm saying to myself what the code is doing when reviewing the patch :)

Good catch BTW.  Thanks!

Looks good.

We might additionaly want to switch the comments to use ',' as a separator instead of '/' if ',' is clearly the preferred one.

(In reply to comment #5)
> Also, it looks like the branch adds then removes the reader testcases for
> "route3=6:7:8:9:0:1:2:3/126,::,1"; should we add that back in as a valid
> testcase?  If somebody hand-writes the keyfile, I can see them using that
> notation and that should probably be valid.

+1

(In reply to comment #7)
> (In reply to comment #5)
> > Also, it looks like the branch adds then removes the reader testcases for
> > "route3=6:7:8:9:0:1:2:3/126,::,1"; should we add that back in as a valid
> > testcase?  If somebody hand-writes the keyfile, I can see them using that
> > notation and that should probably be valid.
> 
> +1

As far as I understand (talking with thomas on IRC), this format actually happens to be the preferred one (:: being optional).

(In reply to comment #5)

I pushed some !fixup that should address the issues up to now.


> > keyfile: fix reader and writer for writing routes
> 
> in reader.c::build_ip6_address_or_route():
> 
> + if (!metric_str && get_one_int (gateway_str, G_MAXUINT32, NULL, &metric)) {
> +     addr = in6addr_any;
> +     metric_str = NULL;
> 
> can remove the metric_str = NULL there since it was already checked for NULL
> just above.

I don't agree. I even extended the check when to try the workaround. For example, before my last fixup, it would also accept an the address "address1=a:b:c:d::/64,5" (which would be wrong). Without !metric_str it would read "route1=a:b:c:d::/64,4,5" as "route1=a:b:c:d::/64,::,4" instead of failing with error.


> Also, it looks like the branch adds then removes the reader testcases for
> "route3=6:7:8:9:0:1:2:3/126,::,1"; should we add that back in as a valid
> testcase?  If somebody hand-writes the keyfile, I can see them using that
> notation and that should probably be valid.

Commit "keyfile: add test cases for reading route and addresses" adds two tests:
+#route3=6:7:8:9:0:1:2:3/126,,1
+route3=6:7:8:9:0:1:2:3/126,::,1
+route4=7:8:9:0:1:2:3:4/125/::,5
With route3 I would like to test the commented-out syntax, but before the following patch, that syntax is not yes supported. So, at first, route3 and route4 test the very same syntax (because the reader is agnostic to the delimiter). Only after "keyfile: fix reader and writer for writing routes" I uncomment what I really want to test with route3. 


> writer.c::write_ip4_values():
> 
> I don't think we need the (unsigned long) cast for plen at all?  The prefix
> will never be > 128, and the 'plen' variable is a 'guint32', so it shouldn't
> need a long conversion at all.  I think we can go with just "%u" here and no
> cast.  Same thing for 'metric' too.

%u expects an unsigned int and the argument is guint32. C promotes some types (char, short, ...) to int. But still, it depends on the compiler/platform whether guint32 gets really passed over as an unsigned int. With this cast it is always correct. I just changed the format for plen from %lu to %u (you are right, no overflow possible there because plen is <= 128).

> Also, instead of "allow_omitting_specified_gateway", perhaps it should be
> "force_write_gateway"?  That would change the if() in
> ip6_array_to_addr_prefix() to a clearer:
> 
>  if (force_write_gateway || !IN6_IS_ADDR_UNSPECIFIED())
> 
> The parameter's intention is to force writing the gateway or to leave it up to
> IN6_IS_ADDR_UNSPECIFIED(), and I think 'force_write_gateway' is a clearer name
> here.  At least in my head, when I'm saying to myself what the code is doing
> when reviewing the patch :)

Agree, this double negative was confusing.

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > Also, it looks like the branch adds then removes the reader testcases for
> > > "route3=6:7:8:9:0:1:2:3/126,::,1"; should we add that back in as a valid
> > > testcase?  If somebody hand-writes the keyfile, I can see them using that
> > > notation and that should probably be valid.
> > 
> > +1
> 
> As far as I understand (talking with thomas on IRC), this format actually
> happens to be the preferred one (:: being optional).

Indeed, "6:7:8:9:0:1:2:3/126,::,1" will be the preferred format. This is already tested by route4. Note that this form gets tested as route4 (route3 is there to test the other form).

(In reply to comment #9)
> (In reply to comment #5)
> 
> I pushed some !fixup that should address the issues up to now.
> 
> 
> > > keyfile: fix reader and writer for writing routes
> > 
> > in reader.c::build_ip6_address_or_route():
> > 
> > + if (!metric_str && get_one_int (gateway_str, G_MAXUINT32, NULL, &metric)) {
> > +     addr = in6addr_any;
> > +     metric_str = NULL;
> > 
> > can remove the metric_str = NULL there since it was already checked for NULL
> > just above.
> 
> I don't agree. I even extended the check when to try the workaround. For
> example, before my last fixup, it would also accept an the address
> "address1=a:b:c:d::/64,5" (which would be wrong). Without !metric_str it would
> read "route1=a:b:c:d::/64,4,5" as "route1=a:b:c:d::/64,::,4" instead of failing
> with error.

I mean make this patch on top:

 if (!metric_str && get_one_int (...)) {
     addr = in6addr_any;
-    metric_str = NULL;

since the if() can only run if metric_str == NULL, there's no point in setting it to NULL inside the if() too.

Rest of the fixups look OK to me.

(In reply to comment #11)

>  if (!metric_str && get_one_int (...)) {
>      addr = in6addr_any;
> -    metric_str = NULL;

Oh yes! pushed a !fixup.

Looks good to me then.

pushed to upstream master:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9af77c570e50a0ae91f1fee4377beef4a56cbc10
http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=376aa50f5ab21b10a0f6cf7e861383f1b49db1f9
http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e3f89eb43267e9ca62171c046e12003951b4ba36