NM bugzilla reorganization. Sorry for the bug spam.

(In reply to comment #0)
> Also consider if there are use cases where 'gateways' option with multiple
> gateways could be of any use or not.

Dynamic IPv6 configuration definitely uses multiple gateways but I don't know how it works as according to netlink, network+plen+metric constitute a unique key.

Normaly there would only be one gateway.

(In reply to comment #2)
> Normaly there would only be one gateway.

So, for the beginning I would use a single gateway as otherwise we would have to set up priorities and those could clash with other priority stuff. The same would apply for libndp from which we would just select one gateway. Any objections?

Now that router discovery is being done in userspace it's pretty clear that from all dynamic default route configuration methods we *always* install just one default route (per protocol version), even though each of the dynamic protocols can provide multiple gateways.

For static configurations, multiple default gateways would only be useful if we check the reachability of the gateways somehow. That still doesn't answer the question whether it makes sense to have multiple gateways or not.

*** Bug 682943 has been marked as a duplicate of this bug. ***

(In reply to comment #4)
> That still doesn't answer the
> question whether it makes sense to have multiple gateways or not.

It does not. Note that the daemon doesn't even support multiple gateways anyway; you can set a different gateway on each address, but NMIP4Config / NMIP6Config only use one of them. (The first non-0 one.)

If you need more than one "gateway", you can add them as routes (and then you get to specify metrics too).

Also, in the case of IP6, the gateway property would almost always be unset, since you would normally expect to get it from router discovery even when you're setting static IP addresses as well.



In addition to removing the gateways from the :addresses properties, we also want to add additional per-address information (eg, labels). And I think it makes sense to do this as a single change, so that we're only reorganizing the address properties once.

As seen in the 6.5 interface aliases patch, storing address information in a parallel array (eg, :addresses, :address-labels) is tricky, because it's hard to figure out what the caller intends in all situations (ie, if they change the value of :addresses, it's hard to know if they're "editing" it [and so :address-labels should be preserved] or if they're "replacing" it [and so :address-labels should be cleared]).

So a better way to do this would be to add a new property that contains the new information *and* all of the old information. Then the old :addresses property would just be a "view" of the new property that only sees some of the information. (And if you set the old property, you lose all of the new information.)


So, we'd have a :gateway property (which would be a string containing an IP address rather than being binary, because the string form is more D-Bus friendly), and an array-valued property called, say, :address-data. Each element of that array would be a struct containing:

  char       *ip_address
  guint8      prefix;
  GHashTable *additional_info; /* string->string */

with the additional_info being the extension point. Particular additional_info might include "label", and "gateway" (for backward compatibility with old configurations that specified multiple addresses with multiple gateways. The gateway for the first address would always be stored in the :gateway property though, not as part of the address data).

This structure would also get used from NMIP4Config/NMIP6Config, where it could be used to export additional information to clients. (Eg, address lifetime, a boolean flag indicating which addresses were user-provided and which were dynamically assigned, etc.

(In reply to comment #6)
> (In reply to comment #4)
> > That still doesn't answer the
> > question whether it makes sense to have multiple gateways or not.
> 
> It does not. Note that the daemon doesn't even support multiple gateways
> anyway; you can set a different gateway on each address, but NMIP4Config /
> NMIP6Config only use one of them. (The first non-0 one.)

+1

> If you need more than one "gateway", you can add them as routes

We generally don't add default routes (plen=0) to the list of routes.

> (and then you get to specify metrics too).

If you specify the metric, then you could just as well only add the route with the best metric and ignore the others. I don't see any real use case for metrics in kernel other than interoperability between multiple configuration tools speaking to the kernel.

> Also, in the case of IP6, the gateway property would almost always be unset,
> since you would normally expect to get it from router discovery even when
> you're setting static IP addresses as well.

That's like saying the same for IPv4 and DHCP. In both cases the default method is automatic but especially server use cases sometimes require static address and routing configuration (be it for IPv4 or IPv6).

> In addition to removing the gateways from the :addresses properties, we also
> want to add additional per-address information (eg, labels). And I think it
> makes sense to do this as a single change, so that we're only reorganizing the
> address properties once.

You are right that removing the addresses and adding other information should be done together as part of what we refer to as NetworkManager 0.10. On the other hand, the 'gateway' option itself can be added for 0.9.x without breaking compatibility.

> As seen in the 6.5 interface aliases patch, storing address information in a
> parallel array (eg, :addresses, :address-labels) is tricky, because it's hard
> to figure out what the caller intends in all situations (ie, if they change the
> value of :addresses, it's hard to know if they're "editing" it [and so
> :address-labels should be preserved] or if they're "replacing" it [and so
> :address-labels should be cleared]).

Makes sense.

> So a better way to do this would be to add a new property that contains the new
> information *and* all of the old information. Then the old :addresses property
> would just be a "view" of the new property that only sees some of the
> information. (And if you set the old property, you lose all of the new
> information.)

Sounds like a good contingency plan.

> So, we'd have a :gateway property (which would be a string containing an IP
> address rather than being binary, because the string form is more D-Bus
> friendly), and an array-valued property called, say, :address-data.

Or addresses2, address-list and so on.

> Each
> element of that array would be a struct containing:
> 
>   char       *ip_address
>   guint8      prefix;
>   GHashTable *additional_info; /* string->string */

Makes sense.

> with the additional_info being the extension point. Particular additional_info
> might include "label", and "gateway" (for backward compatibility with old
> configurations that specified multiple addresses with multiple gateways.

I don't think we need to expose gateway for addresses in the new API at all.

> The
> gateway for the first address would always be stored in the :gateway property
> though, not as part of the address data).

Therefore you only need to project it in the old API, right?

> This structure would also get used from NMIP4Config/NMIP6Config, where it could
> be used to export additional information to clients.

I was even considering a separate header file and address/route/gateway types shared among a couple of modules including ip configs, platform and rdisc. But I'm not yet sure whether it's a good idea or not. But ip configs already use platform's data structures.

> (Eg, address lifetime, a
> boolean flag indicating which addresses were user-provided and which were
> dynamically assigned, etc.

Since we [currently] only use lifetime to check whether an address is dynamic, the flag would be redundant.

All in all, the idea of extending the API in this way sounds good to me.

(In reply to comment #7)
> > Also, in the case of IP6, the gateway property would almost always be unset,
> > since you would normally expect to get it from router discovery even when
> > you're setting static IP addresses as well.
> 
> That's like saying the same for IPv4 and DHCP. In both cases the default method
> is automatic but especially server use cases sometimes require static address
> and routing configuration (be it for IPv4 or IPv6).

In IPv4-land, when you are configuring static addresses, you are usually configuring *only* static addresses, and you need to manually specify the gateway as well, because there's no other way to find it. But in IPv6-land, in general, you are configuring static addresses *in addition to* to the SLAAC-configured address, and getting the gateway from SLAAC. It is almost always possible to correctly autodetect the IPv6 gateway.

> > This structure would also get used from NMIP4Config/NMIP6Config, where it could
> > be used to export additional information to clients.
> 
> I was even considering a separate header file and address/route/gateway types
> shared among a couple of modules including ip configs, platform and rdisc. But
> I'm not yet sure whether it's a good idea or not. But ip configs already use
> platform's data structures.

They use the platform data structures internally, but they'd convert them to the new type for exporting the data over D-Bus.

(In reply to comment #8)
> (In reply to comment #7)
> > > Also, in the case of IP6, the gateway property would almost always be unset,
> > > since you would normally expect to get it from router discovery even when
> > > you're setting static IP addresses as well.
> > 
> > That's like saying the same for IPv4 and DHCP. In both cases the default method
> > is automatic but especially server use cases sometimes require static address
> > and routing configuration (be it for IPv4 or IPv6).
> 
> In IPv4-land, when you are configuring static addresses, you are usually
> configuring *only* static addresses, and you need to manually specify the
> gateway as well, because there's no other way to find it. But in IPv6-land, in
> general, you are configuring static addresses *in addition to* to the
> SLAAC-configured address, and getting the gateway from SLAAC.

Your mileage may vary.

> It is almost always possible to correctly autodetect the IPv6 gateway.

Even with your interpretation the word 'almost' is of key importance here. We both agree we need to have 'gateway' for both IPv4 and IPv6 and that it will have the very same meaning, right? We only disagree on how often each of those will be used in practice, which should not be a problem.

> > > This structure would also get used from NMIP4Config/NMIP6Config, where it could
> > > be used to export additional information to clients.
> > 
> > I was even considering a separate header file and address/route/gateway types
> > shared among a couple of modules including ip configs, platform and rdisc. But
> > I'm not yet sure whether it's a good idea or not. But ip configs already use
> > platform's data structures.
> 
> They use the platform data structures internally, but they'd convert them to
> the new type for exporting the data over D-Bus.

I was talking about the internal structures, here.

I agree with the approach from comment 6 though the issue of multiple gateways is still outstanding.  However, we don't have to address this at that time since we can include the gateway key in the 'additional_info' for now.

For gateways, allowing the default route to be entered directly implies that it should always exist in the routing table while the connection is up (like other routes).  This creates ambiguity with automatically managed routes; if the user explicitly specifies the gateway as a default route, does that override any automatic NM gateway?  This needs to be more thought out and done in conjunction with device/connection priorities which is another bug.

(Plus, entering the gateway as a route doesn't match up with what other OSs do for the gateway, which is simply an entry box.)

All that just to say I think we should stay with the status-quo for now, eg gateways in the addresses, *or* have a separate 'gateways' property that only contains gateway addresses.  We shouldn't defer gateways to routes at this time without a larger plan for handling stuff like multi-hop, multiple gateways, and connection priorities.

(In reply to comment #10)
> I agree with the approach from comment 6 though the issue of multiple gateways
> is still outstanding.  However, we don't have to address this at that time
> since we can include the gateway key in the 'additional_info' for now.

It's roughly the same as continuing to use the special 'gateway' field for addresses, it still keeps the gateway wrongly as part of the address.

> For gateways, allowing the default route to be entered directly implies that it
> should always exist in the routing table while the connection is up (like other
> routes). 

Correct. The dependency between routes in general (including default and non-default routes) should be handled but that's also a separate issue. Currently, if you have device routes and 'via' routes together, we don't handle the ordering. The safest way could be (feel free to copy to a new bug report) to process routes in this order:

1) non-default device routes
2) default device route
3) non-default gateway routes
4) default gateway route

If we come to a conclusion that non-default routes never depend on the default device route, then #2 can be merged into #4 which would make it easier (default route always handled at the end whether it's a device route or a gateway route).

Also, later we should decide whether we allow manually specified routes to depend on automatically configured routes (e.g. manually override a gateway where DHCP address with plen is used). In that case the route setting must be deferred until the respective device route is in place.

> This creates ambiguity with automatically managed routes; if the user
> explicitly specifies the gateway as a default route, does that override any
> automatic NM gateway?

You only have a single default gateway in kernel. Multiple gateways were (IMO wrongly) used by kernel autoconfiguration. Also multiple gateways could *theoretically* be used with different metrics but it's impractical as that means only one gateway is actually used.

> (Plus, entering the gateway as a route doesn't match up with what other OSs do
> for the gateway, which is simply an entry box.)
> ...
> *or* have a separate 'gateways' property that only
> contains gateway addresses.

Good with me, except that I still don't see an actual use for multiple gateways unless NetworkManager converts them to one kernel default route with multiple nexthop gateways specified.

If we plan to support multiple next hops through a single interface, a list of gateways would make sense, even if we stared with just always using the first one.

(In reply to comment #10)
> (Plus, entering the gateway as a route doesn't match up with what other OSs do
> for the gateway, which is simply an entry box.)

Right, I was saying we should have a single "gateway" property (with a single entry in the connection editor, just like other OSes). The "specify gateways as routes" thing was only for the case where you wanted to specify multiple gateways.

And in fact, you *already* have to do that; while NM lets you specify multiple gateways in the connection editor, it completely ignores all of them except the first one. So that's not even a change (other than the fact that we would no longer be pretending to support multiple gateways when we don't really).

ie, if you currently have this:

  ADDRESSES:
    Address        Netmask         Gateway
    192.168.0.5    255.255.255.0   192.168.0.1
    192.168.0.6    255.255.255.0
    10.0.50.20     255.0.0.0       10.0.0.1     <-- NM ignores this gateway

in the new world order you'd have:

  ADDRESSES:
    Address        Prefix
    192.168.0.5        24
    192.168.0.6        24
    10.0.50.20          8

  GATEWAY: 192.168.0.1

And, in either the old or the new system, if the admin wanted there to be a route via 10.0.0.1, they'd have to specify that as a route, not a gateway

(In reply to comment #12)
>   ADDRESSES:
>     Address        Prefix
>     192.168.0.5        24
>     192.168.0.6        24
>     10.0.50.20          8
> 
>   GATEWAY: 192.168.0.1
> 
> And, in either the old or the new system, if the admin wanted there to be a
> route via 10.0.0.1, they'd have to specify that as a route, not a gateway

I sort of think I understand all points of view now. I agree with both of you that the gateway(s) should be separate from addresses. I agree with Dan Winship that it is almost never useful to have multiple gateways.

So the main question is, what is the use case for multiple gateway values. Whether it's multiple gateways specified as nexthops for one route (and why?) or anything else. I would rule out multiple gateway routes with different metrics as that's useless (only one is used in that case), and I would rule out multiple gateway routes with the same metric as it's clearly erroneus.

I think we should seek advice of someone knowlegdeable of real use cases for single-interface multi-nexthop routes if there's one at all. I believe we can live with a single 'gateway' option.

While I understand Dan Winship's argument about still being able to use the list of routes for such corner cases, I don't think we need to care about it until we actually start supporting those corner cases.

I have a real use case for single interfaces with multiple routes, where not all the routes have the same next hop: I have a server that has a network provider who has asked me to that.  (I think the most common reason they'll ask for it is if they want us test a particular route on a different gateway.)

I am unlikely to ever run NM on that box until and unless NM becomes a lot more flexible for server usecases, so my experience here may be largely moot.

(In reply to comment #14)
> I have a real use case for single interfaces with multiple routes,
> where not all the routes have the same next hop:

That makes me curious...

> I have a server that has a network provider who has asked me to that.

That's not a use case. You would have to specify what exactly they expected from such a step.

The Linux kernel's Netlink interface doesn't allow you to add two routes which only differ in the next hop. You would have to add *one* route with *multiple* next hops over the same interface. But even then we still miss the real use case.

> I think the most common reason they'll ask
> for it is if they want us test a particular route on a different gateway.

That's too vague. You cannot test much by setting multiple gateways. It could have been a bad guess from you, or it could have been a mistake on their side.

Multiple next hops in the default route are only useful for multipath routing. We are specifically talking about multiple gateways with one interface, i.e. all next hops would share one interface. That's only useful if you have a single ethernet segment with multiple routers to be used in a round-robin fashion. That's pretty uncommon.

More information:

http://tools.ietf.org/html/rfc2991
http://tools.ietf.org/html/rfc2992
http://www.embeddedlinux.org.cn/linux_net/0596002556/understandlni-CHP-31-SECT-2.html

So we are still choosing from roughly two possibilities:

1) Support multiple gateways per interface in configuration. Support (at least) single-interface multi-gateway routes in nm-platform. Turn each gateway (together with the same ifindex) into a nexthop and build a default route with those nexthops. Let the users enter multiple gateway IP addresses in all of the user interfaces.

1b) Only use the first gateway as long as multi-nexthop routes are not supported internally. Optionally let the user enter only one address in some/all of the interfaces.

2) Support only one gateway per interface in configuration. Let the user enter only one gateway IP address in all of the user interafaces.

> 
> The Linux kernel's Netlink interface doesn't allow you to add two routes which
> only differ in the next hop. You would have to add *one* route with *multiple*
> next hops over the same interface. But even then we still miss the real use
> case.

Huh?  As an example:

ip addr add 10.0.0.1/24 dev em1
ip route add 10.0.1.0/24 dev em1 via 10.0.1.2
ip route add 10.0.2.0/24 dev em1 via 10.0.1.3

That's one interface with one IPv4 address and *two* routes.  My provider might do this because the route to 10.0.2.0/24 is experimental or because it really will use a separate router in production.

There is no such thing as "*one* route with *multiple* next hops".

Please don't offer support multiple *gateways* as such -- I don't think anyone will understand what it means.  Usually a "gateway" is just shorthand for a default route via the "gateway" as the next hop.  It makes sense if there's one, but it's unclear what's supposed to happen if there are two.

Your example has a typo ('via' must lie somewhere inside of 10.0.0.0/24).

But more importantly, it is not an example for "add two routes which only differ in the next hop".

An example for that would be:

# ip addr add 10.0.0.1/24 dev em1
# ip route add 10.0.1.0/24 dev em1 via 10.0.0.2
# ip route add 10.0.1.0/24 dev em1 via 10.0.0.3
RTNETLINK answers: File exists

which does not work, as Pavel said.

Setting up multiple routes to *different* subnets works with NetworkManager, of course.

In that case I misunderstood the original question.  Please ignore my comment.

(Note: that was indeed a typo, although it is possible on Linux to have the next hop be outside the subnet.  All you need is for the address to be local, which can be arranged by explicitly making it local, like this:

$ sudo ip route add local 1.2.3.4/32 dev wlan0
$ sudo ip route add 1.2.3.5/32 dev wlan0 via 1.2.3.4

I even had to do that once.  It was a strange use case, and I doubt that NM will ever be asked to support that.)

pushed danw/addressdata-bgo682946

Main highlights:

  - merged NMIP4Address+NMIP6Address and NMIP4Route+NMIP6Route. The new
    types represent IP addresses as strings, and allow arbitrary additional
    "attributes". For use in NMSettings, the only defined attribute is
    "label" on NMIPAddress, but for NMIP4Config/NMIP6Config (which I
    probably should have also merged...), I threw in a few bonus
    attributes like "lifetime" and "peer_address" as examples.

    (The attribute data is serialized over D-Bus via new "address-data"
    and "route-data" properties, but this fact is completely hidden from
    the libnm API.)

  - NMSettingIP4Config and NMSettingIP6Config are now subclasses of
    NMSettingIPConfig, with almost all of their API moved to the parent
    class.

  - NMSettingIPConfig has a new "gateway" property, which stores the
    gateway IP address as a string

  - NMIPAddress does NOT include a gateway; the only gateway a
    configuration has is through its NMSettingIPConfig:gateway property.

      - This results in absolutely no loss of functionality, since, as
        mentioned before, NM has *always* only supported a single gateway,
        and completely ignored any additional gateways specified in the
        config.

      - The serialization of NMSettingIP4Config and NMSettingIP6Config
        remains backward-compatible; the :gateway property gets merged
        into the :addresses property when serializing to D-Bus, and
        gets extracted from it (if it wasn't provided explicitly) when
        deserializing from D-Bus.

      - The settings backends no longer attempt to read and write
        multiple gateways, which required updating some of the tests a
        bit. (Again, this is not a loss of functionality, since the
        additional gateways they previously read and wrote were never
        used for anything.)

Two unfinished bits:

  - nmcli address/gateway handling needs a redesign

  - NMSettingIP4Config:dhcp-send-hostname... what's up with that? Why
    does IPv4 need the ability to have a non-null :dhcp-hostname which
    does not get sent, but IPv6 does not? (IPv6 has :dhcp-hostname
    but not :dhcp-send-hostname.)

    I ran into this because :dhcp-hostname got moved to NMSettingIPConfig
    since it's shared, but its docs refer to the ipv4-specific
    :dhcp-send-hostname property...

(In reply to comment #19)
> pushed danw/addressdata-bgo682946

This is great. Interesting that I just had a related issue these days. Would be great if that could be tackled too...

As far as the kernel is concerned, route metrics are guint32 with a range from 0 to G_MAXUINT32. IPv6 treats the route 0 special, in that the kernel coerces it to 1024.

In NMSettingIP.Config we treat a metric of 0 as "fallback to default priority". That is unfortunate, because it clashes with the valid value "0" (even in the IPv6 case, because 0 doesn't mean "default metric of the device", it means 1024).

We should fix that, so that we use guint64 as type for it, with a valid range -1 to G_MAXUINT32. -1 meaning default and being the default value.

I have pushed a few fixup commits to the branch.

However, there is a problem of corrupting memory somewhere. Running 'nmcli con' or C example for getting connections results in segmentation fault.

*** glibc detected *** /home/jklimes/work/NetworkManager/examples/C/glib/.libs/list-connections-libnm: invalid fastbin entry (free): 0x0000000000689900 ***

Running list-connections-libnm with valgrind gives this:
jklimes@gromit ~$ LD_LIBRARY_PATH=/home/jklimes/work/NetworkManager/libnm/.libs valgrind /home/jklimes/work/NetworkManager/examples/C/glib/.libs/list-connections-libnm 
==22566== Memcheck, a memory error detector
==22566== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==22566== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==22566== Command: /home/jklimes/work/NetworkManager/examples/C/glib/.libs/list-connections-libnm
==22566== 
==22566== Invalid read of size 4
==22566==    at 0x3E9A437D69: g_hash_table_unref (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x4C8D7DF: nm_ip_address_unref (nm-setting-ip-config.c:134)
==22566==    by 0x3E9A41E22A: g_ptr_array_foreach (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A41E2EF: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x4C92B6E: ip4_route_data_set (nm-setting-ip4-config.c:439)
==22566==    by 0x4CA70AA: _nm_setting_new_from_dbus (nm-setting.c:818)
==22566==    by 0x4C72B96: nm_connection_replace_settings (nm-connection.c:324)
==22566==    by 0x4C6489C: replace_settings (nm-remote-connection.c:560)
==22566==    by 0x4C64C6F: init_sync (nm-remote-connection.c:651)
==22566==    by 0x4C609BA: _nm_object_create (nm-object.c:706)
==22566==    by 0x4C61AEA: handle_object_array_property (nm-object.c:1140)
==22566==    by 0x4C61EB7: handle_property_changed (nm-object.c:1214)
==22566==  Address 0x5e29750 is 16 bytes before a block of size 7 free'd
==22566==    at 0x4A07786: free (vg_replace_malloc.c:446)
==22566==    by 0x3E9A44D50E: g_free (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A479C37: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A47A848: g_variant_iter_next (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x4CAD617: nm_utils_ip_routes_from_variant (nm-utils.c:1872)
==22566==    by 0x4C92B3D: ip4_route_data_set (nm-setting-ip4-config.c:437)
==22566==    by 0x4CA70AA: _nm_setting_new_from_dbus (nm-setting.c:818)
==22566==    by 0x4C72B96: nm_connection_replace_settings (nm-connection.c:324)
==22566==    by 0x4C6489C: replace_settings (nm-remote-connection.c:560)
==22566==    by 0x4C64C6F: init_sync (nm-remote-connection.c:651)
==22566==    by 0x4C609BA: _nm_object_create (nm-object.c:706)
==22566==    by 0x4C61AEA: handle_object_array_property (nm-object.c:1140)
==22566== 
==22566== Invalid read of size 4
==22566==    at 0x3E9A437D71: g_hash_table_unref (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x4C8D7DF: nm_ip_address_unref (nm-setting-ip-config.c:134)
==22566==    by 0x3E9A41E22A: g_ptr_array_foreach (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A41E2EF: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x4C92B6E: ip4_route_data_set (nm-setting-ip4-config.c:439)
==22566==    by 0x4CA70AA: _nm_setting_new_from_dbus (nm-setting.c:818)
==22566==    by 0x4C72B96: nm_connection_replace_settings (nm-connection.c:324)
==22566==    by 0x4C6489C: replace_settings (nm-remote-connection.c:560)
==22566==    by 0x4C64C6F: init_sync (nm-remote-connection.c:651)
==22566==    by 0x4C609BA: _nm_object_create (nm-object.c:706)
==22566==    by 0x4C61AEA: handle_object_array_property (nm-object.c:1140)
==22566==    by 0x4C61EB7: handle_property_changed (nm-object.c:1214)
==22566==  Address 0x5e29750 is 16 bytes before a block of size 7 free'd
==22566==    at 0x4A07786: free (vg_replace_malloc.c:446)
==22566==    by 0x3E9A44D50E: g_free (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A479C37: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A47A848: g_variant_iter_next (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x4CAD617: nm_utils_ip_routes_from_variant (nm-utils.c:1872)
==22566==    by 0x4C92B3D: ip4_route_data_set (nm-setting-ip4-config.c:437)
==22566==    by 0x4CA70AA: _nm_setting_new_from_dbus (nm-setting.c:818)
==22566==    by 0x4C72B96: nm_connection_replace_settings (nm-connection.c:324)
==22566==    by 0x4C6489C: replace_settings (nm-remote-connection.c:560)
==22566==    by 0x4C64C6F: init_sync (nm-remote-connection.c:651)
==22566==    by 0x4C609BA: _nm_object_create (nm-object.c:706)
==22566==    by 0x4C61AEA: handle_object_array_property (nm-object.c:1140)
==22566== 

valgrind: m_mallocfree.c:294 (get_bszB_as_is): Assertion 'bszB_lo == bszB_hi' failed.
valgrind: Heap block lo/hi size mismatch: lo = 127, hi = 8388608.
This is probably caused by your program erroneously writing past the
end of a heap block and corrupting heap metadata.  If you fix any
invalid writes reported by Memcheck, this assertion failure will
probably go away.  Please try that before reporting this as a bug.

==22566==    at 0x3805754F: report_and_quit (m_libcassert.c:235)
==22566==    by 0x38057692: vgPlain_assert_fail (m_libcassert.c:309)
==22566==    by 0x38063E7D: vgPlain_arena_free (m_mallocfree.c:292)
==22566==    by 0x380291C5: create_MC_Chunk (mc_malloc_wrappers.c:165)
==22566==    by 0x38029A48: vgMemCheck_realloc (mc_malloc_wrappers.c:533)
==22566==    by 0x3809DA0B: vgPlain_scheduler (scheduler.c:1673)
==22566==    by 0x380AD0C9: run_a_thread_NORETURN (syswrap-linux.c:103)

sched status:
  running_tid=1

Thread 1: status = VgTs_Runnable
==22566==    at 0x4A08A0E: realloc (vg_replace_malloc.c:662)
==22566==    by 0x3E9A44D4A6: g_realloc (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A465176: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A466524: g_string_append_vprintf (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A466726: g_string_append_printf (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9B8BCF3E: g_dbus_connection_signal_subscribe (in /usr/lib64/libgio-2.0.so.0.3200.4)
==22566==    by 0x3E9B8C6CF8: ??? (in /usr/lib64/libgio-2.0.so.0.3200.4)
==22566==    by 0x3E9B8C701F: ??? (in /usr/lib64/libgio-2.0.so.0.3200.4)
==22566==    by 0x3E9B85936E: g_initable_new_valist (in /usr/lib64/libgio-2.0.so.0.3200.4)
==22566==    by 0x3E9B859458: g_initable_new (in /usr/lib64/libgio-2.0.so.0.3200.4)
==22566==    by 0x4C48626: _nm_dbus_new_proxy_for_connection (nm-dbus-helpers.c:280)
==22566==    by 0x4C5F52F: init_sync (nm-object.c:189)
==22566==    by 0x4C64C20: init_sync (nm-remote-connection.c:642)
==22566==    by 0x4C609BA: _nm_object_create (nm-object.c:706)
==22566==    by 0x4C61AEA: handle_object_array_property (nm-object.c:1140)
==22566==    by 0x4C61EB7: handle_property_changed (nm-object.c:1214)
==22566==    by 0x4C61FFC: process_properties_changed (nm-object.c:1246)
==22566==    by 0x4C62DAF: _nm_object_reload_properties (nm-object.c:1452)
==22566==    by 0x4C5F5E4: init_sync (nm-object.c:205)
==22566==    by 0x4C46F3C: init_sync (nm-client.c:1743)
==22566==    by 0x3E9B85936E: g_initable_new_valist (in /usr/lib64/libgio-2.0.so.0.3200.4)
==22566==    by 0x3E9B859458: g_initable_new (in /usr/lib64/libgio-2.0.so.0.3200.4)
==22566==    by 0x4C4690A: nm_client_new (nm-client.c:1580)
==22566==    by 0x400DFF: main (list-connections-libnm.c:77)

Thread 2: status = VgTs_WaitSys
==22566==    at 0x3C472E8BDF: poll (poll.c:87)
==22566==    by 0x3E9A447AF3: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9A447F51: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3E9B8C94D5: ??? (in /usr/lib64/libgio-2.0.so.0.3200.4)
==22566==    by 0x3E9A46A494: ??? (in /usr/lib64/libglib-2.0.so.0.3200.4)
==22566==    by 0x3C47E07D13: start_thread (pthread_create.c:309)
==22566==    by 0x3C472F168C: clone (clone.S:115)

(In reply to comment #20)
> In NMSettingIP.Config we treat a metric of 0 as "fallback to default priority".
> That is unfortunate, because it clashes with the valid value "0" (even in the
> IPv6 case, because 0 doesn't mean "default metric of the device", it means
> 1024).
> 
> We should fix that, so that we use guint64 as type for it, with a valid range
> -1 to G_MAXUINT32. -1 meaning default and being the default value.

Hm... we can't change the semantics of the D-Bus ipv4.routes and ipv6.routes properties, but I guess we could change .route-data (and the APIs) to work that way (and just convert both "0" and "-1" to "0" in the backward-compatibliity .routes property)

Oh, that reminds me of another unfinished thing: the D-Bus settings API is now starting to diverge more from the libnm-util API and we need to figure out how to make the documentation work...

(In reply to comment #21)
> I have pushed a few fixup commits to the branch.

I rebased to master (after the bgo735900-dbus-errors merge), and squashed your two simple fixups, and reordered the other two into place but didn't merge them yet.

With the current tip of that branch, I'm not seeing any memory corruption in either "nmcli con" or list-connections-libnm... are you still seeing it?

(In reply to comment #22)
> (In reply to comment #20)
> > In NMSettingIP.Config we treat a metric of 0 as "fallback to default priority".
> > That is unfortunate, because it clashes with the valid value "0" (even in the
> > IPv6 case, because 0 doesn't mean "default metric of the device", it means
> > 1024).
> > 
> > We should fix that, so that we use guint64 as type for it, with a valid range
> > -1 to G_MAXUINT32. -1 meaning default and being the default value.
> 
> Hm... we can't change the semantics of the D-Bus ipv4.routes and ipv6.routes
> properties, but I guess we could change .route-data (and the APIs) to work that
> way (and just convert both "0" and "-1" to "0" in the backward-compatibliity
> .routes property)

Agree. I meant to preserve the old route property on DBUS level (including the broken meaning of the metric), and adding a new field that can be used by NM and updated clients.

> (In reply to comment #21)
> > I have pushed a few fixup commits to the branch.
> 
> I rebased to master (after the bgo735900-dbus-errors merge), and squashed your
> two simple fixups, and reordered the other two into place but didn't merge them
> yet.
> 
> With the current tip of that branch, I'm not seeing any memory corruption in
> either "nmcli con" or list-connections-libnm... are you still seeing it?

I was going to say I didn't see it. But after running NetworkManager from the branch, the bug appeared again.

Fortunately (after struggling) I have found the problem and pushed the fix. :)

(In reply to comment #24)
> Fortunately (after struggling) I have found the problem and pushed the fix. :)

oops, i made that same mistake in like 4 places (but had already caught the other 3)

So your nmcli patch is marked "--wip--"... what's left to be done there?

danw/addressdata-bgo682946 fails to build:

make[3]: Entering directory `/data/src/NetworkManager/docs/libnm'
  DOC   Building HTML
warning: failed to load external entity "../xml/nm-ip4-config.xml"
../libnm-docs.xml:130: element include: XInclude error : could not load ../xml/nm-ip4-config.xml, and no fallback was found
warning: failed to load external entity "../xml/nm-ip6-config.xml"
../libnm-docs.xml:131: element include: XInclude error : could not load ../xml/nm-ip6-config.xml, and no fallback was found
warning: failed to load external entity "../xml/nm-dhcp4-config.xml"
../libnm-docs.xml:132: element include: XInclude error : could not load ../xml/nm-dhcp4-config.xml, and no fallback was found
warning: failed to load external entity "../xml/nm-dhcp6-config.xml"
../libnm-docs.xml:133: element include: XInclude error : could not load ../xml/nm-dhcp6-config.xml, and no fallback was found
make[3]: *** [html-build.stamp] Error 6



>> libnm-core, all: merge IPv4 and IPv6 address/route types

+    /* We don't accept default routes as NetworkManager handles it itself */
+    if (!strcmp (tmp, "0.0.0.0") || !strcmp (tmp, "::")) {

This is correct, because you canonicallize the address. But NM checks for default routes based on plen==0.
   #define NM_PLATFORM_IP_ROUTE_IS_DEFAULT(route) \
      ( ((const NMPlatformIPRoute *) (route))->plen <= 0 )

I think you should check that plen!=0,
AND that the host-part is zero
AND that the net-part is non-zero


How about adding a property "is_valid" to NMIPRoute and NMIPAddress (possibly with localized GError, so that nmcli can print an error reason).
And there should be nm_ip_route_is_default() which checks does "is_valid()&&plen==0"

(maybe is_valid should be cached internally, so that we don't have to reevaluate it everytime? -- and clear it in any setters).
That would also replace code from NMSettingIP4Config:verify().


I think, we should deliberately check for is_valid(). For example, construct_ip4_items() iterates over addresses and prints nm_ip_address_get_address (addr) without checking that an address is set
(OK, arguably glib printf is save against printing a NULL string).


+static NMIPAddress *
 _parse_ipv4_address (const char *address, GError **error)
 {
-    [SNIP]
+    return _parse_ip_address (AF_INET6, address, error);
                               ^^^^^^^^
 }




include/nm-test-utils.h:

nmtst_assert_ip4_address_equals() can still be useful at other places, for example in libnm-util tests or
src/dhcp-manager/tests/test-dhcp-dhclient.c:test_read_lease_ip4_config_basic()




+void
+nm_ip_address_set_address (NMIPAddress *address,
+                           const char *addr)
+{
+    g_return_val_if_fail (address != NULL, NULL);
+    g_return_val_if_fail (addr != NULL, NULL);
              ^^^
+
+    valid_addr = canonicalize_ip (&addr, address->family, FALSE);
+    g_return_val_if_fail (valid_addr, NULL);
              ^^^



»···if (null_any && !memcmp (&addr_buf, &in6addr_any, sizeof (addr_buf))) {

size should be "family==AF_INET ? 4 : 16"


Can we have family the first argument of canonicalize_ip()? Seems more intuitive to have always family first.


»···if (!*ip) {
»···»···g_return_val_if_fail (null_any, FALSE);
»···»···return TRUE;
»···}

I think, canonicalize_ip() should not g_return depending on the argument it is supposed to check.


»···struct in6_addr addr_buf = IN6ADDR_ANY_INIT;

doesn't need initialization.






I dislike the static buffer in canonicalize_ip(). Can we get rid of it?
Btw. can we accept GCC statement expressions (supported by clang too) and alloca? That would then make:

-canonicalize_ip (const char **ip, int family, gboolean null_any)
+_canonicalize_ip (const char **ip, int family, gboolean null_any, char *canonical)
 {
-    static char canonical[NM_UTILS_INET_ADDRSTRLEN];
     struct in6_addr addr_buf = IN6ADDR_ANY_INIT;
 
     if (!*ip) {
@@ -65,6 +64,12 @@ canonicalize_ip (const char **ip, int family, gboolean null_any)
     *ip = inet_ntop (family, &addr_buf, canonical, sizeof (canonical));
     return TRUE;
 }
+#define canonicalize_ip(ip, family, null_any) \
+    ({ \
+        char *canonical = g_alloca (NM_UTILS_INET_ADDRSTRLEN); \
+        _canonicalize_ip (ip, family, null_any, canonical); \
+    })
 






how about tightening up NMIPAddress, for example, we don't need so many bits for family or prefix.

We can even pack it more using bitfields:


diff --git c/libnm-core/nm-setting-ip-config.c i/libnm-core/nm-setting-ip-config.c
index eab48bd..1670fb3 100644
--- c/libnm-core/nm-setting-ip-config.c
+++ i/libnm-core/nm-setting-ip-config.c
@@ -68,11 +68,16 @@ canonicalize_ip (const char **ip, int family, gboolean null_any)
 
 G_DEFINE_BOXED_TYPE (NMIPAddress, nm_ip_address, nm_ip_address_dup, nm_ip_address_unref)
 
+G_STATIC_ASSERT ( AF_INET >= 0 && AF_INET < 16);
+G_STATIC_ASSERT ( AF_INET6 >= 0 && AF_INET6 < 16);
 struct NMIPAddress {
-    guint refcount;
+    unsigned int refcount : 28;
+
+    /* AF_INET (0x02) and AF_INET6 (0x0A) fit in 4 bit */
+    unsigned int family : 4;
 
-    char *address;
-    int prefix, family;
+    guint8 prefix;
+    char *address, *gateway;
 };
@@ -109,7 +114,7 @@ void
 nm_ip_address_ref (NMIPAddress *address)
 {
     g_return_if_fail (address != NULL);
-    g_return_if_fail (address->refcount > 0);
+    g_return_if_fail (address->refcount > 0 && address->refcount < 1<<28);
 
     address->refcount++;
 }



route metric should be guint32 type.




-    GSList *addresses;  /* array of NMIP4Address */
+    GSList *addresses;  /* array of NMIPAddress */
     GSList *address_labels; /* list of strings */
-    GSList *routes;     /* array of NMIP4Route */
+    GSList *routes;     /* array of NMIPRoute */

todo: could we implement the lists as GPtrArray?


nm_utils_ip4_addresses_from_variant():

maybe it would be useful to have wrappers to pass get/set addresses:
  nm_ip_address_set_address (addr, nm_utils_inet4_ntop (addr_array[0], NULL));
to
  nm_ip_address_set_address_ipv4 (addr, addr_array[0]);
(which would save canonicalize_ip() call).


Also in nm_utils_ip4_addresses_from_variant(), is_valid() would be useful to skip invalid addresses. We already don't trust the other side fully and check:
  if (length < 3) {
but we don't check for invalid prefix.



trailing whitespace in nm_utils_ip6_addresses_to_variant()





+gboolean nm_utils_ipaddr_valid (const char *ip, int family);
+const char *nm_utils_ipaddr_canonical (const char *ip, int family, char *dst);

could we have @family as first parameter?



+    addr = nm_ip_address_new (AF_INET);
+    nm_ip_address_set_address (addr, "2.2.2.2");
+    nm_ip_address_set_prefix (addr, 24);

how about adding nmtst_ip_address_create?




-         guint32 gateway = nm_ip4_address_get_gateway (nm_setting_ip4_config_get_address (setting, i));
+         const char *gateway_str = nm_ip_address_get_gateway (nm_setting_ip4_config_get_address (setting, i));
+         guint32 gateway;
 
-         if (gateway) {
+         if (gateway_str) {
+              inet_pton (AF_INET, gateway_str, &gateway);





similar to before:

guint32 gateway = nm_ip_address_get_gateway_ipv4(...);
if (gateway) {
    ...





read_one_ip4_route():

          errno = 0;
          metric = strtol (value, NULL, 10);
          if (errno || metric < 0) {
               g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION,
                            "Invalid IP4 route metric '%s'", value);
               g_free (value);
               goto out;
          }
-         nm_ip4_route_set_metric (route, (guint32) metric);
+         nm_ip_route_set_metric (route, (guint32) metric);


Was already before: should check valid range. nm_utils_ascii_str_to_int64()?





I would like to see at the end this check:

+    success = nm_ip_route_is_valid (route);
     *out_route = route;
     success = TRUE;

I also think, that a valid route has host-part of network zero. But probably we want to be more relaxed when reading ifcfg files. Hence we would have to mask that off after reading.




 static void
-check_ip6_route (NMSettingIP6Config *config, int idx, const char *destination_str, int plen,
-         const char *next_hop_str, int metric)
+check_ip6_route (NMSettingIP6Config *config, int idx, const char *destination, int plen,
+                 const char *next_hop, int metric)


how about adding
   nmtst_assert_ip_route(route, ...)
and

- check_ip4_route (s_ip4, 2, "1.1.1.2", 12, NULL, 0);
+ nmtst_assert_ip_route_equals (nm_setting_ip4_config_get_route (s_ip4), 2, AF_INET, "1.1.1.2", 12, NULL, 0);

or alternatively:

#define check_ip4_route(s_ip4, index, network, plen, gateway, metric) \
    nmtst_assert_ip_route_equals (nm_setting_ip4_config_get_route (s_ip4), \ 
                                  index, AF_INET, network, plen, gateway, metric);




+              if (!gw) {
+                   if (!strcmp (setting_name, NM_SETTING_IP4_CONFIG_SETTING_NAME))
+                        gw = "0.0.0.0";
+                   else


could we determine the address family above and then check "if (family == AF_INET)"?





>> libnm-core: add NMIPAddress/NMIPRoute attributes, use for labels

maybe nm_ip_address_get_attribute_names() should return NULL if no attributes are set.


The functions should g_return_*if_fail (name != NULL && name[0]);

Maybe add a function nm_ip_address_get_num_attributes()?

There is no function to delete an attribute. Maybe setting the value to NULL means delete attribute. But the current set function does not work like that.


-         const char *label = (const char *) l_iter->data;
+         const char *label = nm_ip_address_get_attribute (addr, "label");

just wondering, do we want some more namespaces there? What is the usecase for these attributes? What names are expected?

Could you make "label" a define? Like NM_IP_ADDRESS_ATTRIBUTE_LABEL




+    _nm_setting_class_add_dbus_only_property (parent_class,
+                                              "address-labels",
+                                              G_VARIANT_TYPE_STRING_ARRAY,
+                                              ip4_address_labels_get,
+                                              NULL);

I though that address-labels were never exposed to the client? E.g. they are dropped from libnm-util.


Also, don't we want now to expose the address attributes via DBUS?
I think the solution to add another string dictionary to our "aau" address
seems wrong. Can we not add instead a address_new field, which has the attributes as member (and strings as addresses).
Mind also, the route-metric default value.


ip4_address_labels_get():

do we always want to transfer address-labels, even if no address has any labels? I think, labels are uncommon, we could optimize here a bit.





>> libnm-core: extract NMSettingIPConfig superclass out of IP4, IP6 classes





 NMIPAddress *
 nm_ip_address_dup (NMIPAddress *address)
 {
     NMIPAddress *copy;
 
-    g_return_if_fail (address != NULL);
-    g_return_if_fail (address->refcount > 0);
+    g_return_val_if_fail (address != NULL, NULL);
+    g_return_val_if_fail (address->refcount > 0, NULL);

could you move these a few commits earlier? (and similar)




+ * #NMSettingIP4Config has no properties or methods of its own; it inherits
+ * everything from #NMSettingIPConfig.

this seems wrong. It has @dhcp_client_id and @dhcp_send_hostname.




-    for (iter = priv->addresses; iter; iter = iter->next) {
-         NMIPAddress *addr = iter->data;
+    for (i = 0; i < nm_setting_ip_config_get_num_addresses (s_ip); i++) {
+         NMIPAddress *addr = nm_setting_ip_config_get_address (s_ip, i);

fetch num_addresses once before the for loop?




(more later)

Could we also make the functions to NMIPRoute and NMIPAddress with the proper const modifiers? E.g. getters accepting a const pointer, setters non-const.

(In reply to comment #26)
> danw/addressdata-bgo682946 fails to build:

oops, it worked with a dirty build tree where the missing files still existed

> >> libnm-core, all: merge IPv4 and IPv6 address/route types
> 
> +    /* We don't accept default routes as NetworkManager handles it itself */
> +    if (!strcmp (tmp, "0.0.0.0") || !strcmp (tmp, "::")) {

> I think you should check that plen!=0,

It does that as well a few lines later

> How about adding a property "is_valid" to NMIPRoute and NMIPAddress (possibly
> with localized GError, so that nmcli can print an error reason).

Currently the assumption is that the caller handles validation; nm_ip_address_set_address(), nm_ip_route_set_dest(), and nm_ip_route_set_next_hop() all fail if you pass an invalid IP address, and nm_ip_address_set_prefix() and nm_ip_route_set_prefix() likewise fail if you pass an invalid prefix.

Maybe it would make sense to change that though?

> And there should be nm_ip_route_is_default() which checks does
> "is_valid()&&plen==0"

plen==0 is an odd case, because (as with the old 4/6-specific types), 0 is the default value for prefix, but it's not a valid value to pass to set_prefix()...

We should probably fix that... it's kind of bizarre.

> I think, we should deliberately check for is_valid(). For example,
> construct_ip4_items() iterates over addresses and prints
> nm_ip_address_get_address (addr) without checking that an address is set
> (OK, arguably glib printf is save against printing a NULL string).

nm_utils_ip4_addresses_from_variant() will only return valid addresses

> nmtst_assert_ip4_address_equals() can still be useful at other places

It *could* be useful, but we're not actually using it there. I think it's better to remove it now that it's unused, and if someone wants to use it in tests again later, they can add it back.

> +    g_return_val_if_fail (addr != NULL, NULL);
>               ^^^

For some reason, gcc never gives me warnings about that... it must be because of -O0 I guess.

> »···if (null_any && !memcmp (&addr_buf, &in6addr_any, sizeof (addr_buf))) {
> 
> size should be "family==AF_INET ? 4 : 16"

We initialize addr_buf to all 0s, so it's safe to check the whole buffer

> Can we have family the first argument of canonicalize_ip()? Seems more
> intuitive to have always family first.

OK, I'd had the same thought at one point but decided against it, but if you think so too then there's probably some logic to it.

Likewise with nm_utils_ipaddr_valid(), for consistency. And I also removed nm_utils_ipaddr_canonical() since nothing actually used it.


> »···if (!*ip) {
> »···»···g_return_val_if_fail (null_any, FALSE);
> »···»···return TRUE;
> »···}
> 
> I think, canonicalize_ip() should not g_return depending on the argument it is
> supposed to check.

It's a programmer error to pass NULL unless null_any is set. All of the original callers tested for that beforehand, but the ones added in "libnm-core: extract NMSettingIPConfig superclass out of IP4, IP6 classes" didn't.

> »···struct in6_addr addr_buf = IN6ADDR_ANY_INIT;
> 
> doesn't need initialization.

as mentioned above, the memcmp later on depends on it having been initialized

> I dislike the static buffer in canonicalize_ip().

It is simple and causes no problems in practice.

> how about tightening up NMIPAddress, for example, we don't need so many bits
> for family or prefix.
> 
> We can even pack it more using bitfields:

That micro-optimizes memory usage at the expense of micro-UN-optimizing access speed (since bitfield operations are going to be slower than word-sized operations).

> -    int prefix, family;
> +    guint8 prefix;
> +    char *address, *gateway;

Note that that change wouldn't actually make the struct smaller, since the compiler will add padding to align address to an 8-byte boundary anyway.

> route metric should be guint32 type.

fixed, though I'm still pondering the int64 thing

> todo: could we implement the lists as GPtrArray?

Yes. It would probably simplify things given that the externally-visible properties are already ptrarrays. But that's all internal and can happen at any time.

> maybe it would be useful to have wrappers to pass get/set addresses:
>   nm_ip_address_set_address (addr, nm_utils_inet4_ntop (addr_array[0], NULL));
> to
>   nm_ip_address_set_address_ipv4 (addr, addr_array[0]);
> (which would save canonicalize_ip() call).

I pushed a commit "[WIP] add nm_ip_address_set_binary(), etc", but I'm not sure it's worth it, since it only gets used in a few places (which was part of the point of changing from binary to strings in the first place; in most places, the string representation is more convenient).

> Also in nm_utils_ip4_addresses_from_variant(), is_valid() would be useful to
> skip invalid addresses. We already don't trust the other side fully and check:
>   if (length < 3) {
> but we don't check for invalid prefix.

mmm... ok, need to do something here still

> +    addr = nm_ip_address_new (AF_INET);
> +    nm_ip_address_set_address (addr, "2.2.2.2");
> +    nm_ip_address_set_prefix (addr, 24);
> 
> how about adding nmtst_ip_address_create?

So... in an earlier version of this branch, I had made NMIPAddress and NMIPRoute immutable, and you had to pass all of the arguments at construct time. I eventually gave up on that, because there are places where the mutability is just very useful. (In particular, clients/tui/nm-editor-bindings.c). And then I tried having it where nm_ip_address_new() still took the address/prefix arguments, but you could also use setters later if you preferred. But then it was weird that in some cases you'd be passing dummy values to the new() function

So... yes, we could add an nmtst function, but maybe we should just add an NMIPAddress function instead? Not sure about the name... nm_ip_address_new_full() maybe?

> how about adding
>    nmtst_assert_ip_route(route, ...)

maybe later...

> >> libnm-core: add NMIPAddress/NMIPRoute attributes, use for labels
> 
> maybe nm_ip_address_get_attribute_names() should return NULL if no attributes
> are set.

No, we made all the other libnm methods *stop* doing that, and it simplified code (by getting rid of a "!= NULL" check at every call site, where the code would already have done the right thing with a 0-length array).

> Maybe add a function nm_ip_address_get_num_attributes()?

There doesn't seem to be a need for it.

> There is no function to delete an attribute. Maybe setting the value to NULL
> means delete attribute. But the current set function does not work like that.

Yes. I fixed it later in the branch, but that belongs here...

> +         const char *label = nm_ip_address_get_attribute (addr, "label");
> 
> just wondering, do we want some more namespaces there? What is the usecase for
> these attributes? What names are expected?

Not sure there are any further expected use cases for NMSettingIPConfig, but there's lots of additional data we can be exporting per-address in NMIPConfig. (as seen later in the branch)

It is NOT intended for "private use" extensions, only for stuff defined by NM itself. So I don't think we need namespacing.

> Could you make "label" a define? Like NM_IP_ADDRESS_ATTRIBUTE_LABEL

Maybe later... for now the label stuff is still a little bit too tied to the fedora/rhel device aliases stuff. (Eg, verify_label() requires that the label be in the form that is used by that.) If we genericize it a bit later, then maybe we can make it more explicit in the API.

> I though that address-labels were never exposed to the client? E.g. they are
> dropped from libnm-util.

They have to be exposed over D-Bus, because when you edit a connection, it gets sent from daemon to client and back, so if the labels weren't serialized, they'd always get dropped when you saved the connection.

They aren't in libnm-util any more because since they were never publicly exposed in the past, we're treating them as though they were post-0.9.10 API, and were never added to libnm-util in the first place.

> Also, don't we want now to expose the address attributes via DBUS?

As I'm sure you noticed later, this does happen in a later commit.

> + * #NMSettingIP4Config has no properties or methods of its own; it inherits
> + * everything from #NMSettingIPConfig.
> 
> this seems wrong. It has @dhcp_client_id and @dhcp_send_hostname.

Oops, yeah, originally I had moved those into NMSettingIPConfig, then forgot to fix the comment.



pushed with a lot of fixups... it's kind of a mess at this point

pushed danw/addressdata-squashed too

pushed on fixup.


>> libnm-core: extract NMSettingIPConfig superclass out of IP4, IP6 classes

+int nm_setting_ip_config_get_num_addresses (NMSettingIPConfig *setting);
+int nm_setting_ip_config_get_num_routes    (NMSettingIPConfig *setting);

make it guint?




> > How about adding a property "is_valid" to NMIPRoute and NMIPAddress (possibly
> > with localized GError, so that nmcli can print an error reason).
> 
> Currently the assumption is that the caller handles validation;
> nm_ip_address_set_address(), nm_ip_route_set_dest(), and
> nm_ip_route_set_next_hop() all fail if you pass an invalid IP address, and
> nm_ip_address_set_prefix() and nm_ip_route_set_prefix() likewise fail if you
> pass an invalid prefix.
> 
> Maybe it would make sense to change that though?

for example, setting an route to 192.168.0.1/8 should be invalid (because the host-part must be zero). Looking at

     nm_ip_route_set_dest (route, "192.168.0.1");
     nm_ip_route_set_prefix (route, 8);

must both succeed (individually). But together the object is invalid. Hence, is_valid() would be very useful IMO.


> > And there should be nm_ip_route_is_default() which checks does
> > "is_valid()&&plen==0"
> 
> plen==0 is an odd case, because (as with the old 4/6-specific types), 0 is the
> default value for prefix, but it's not a valid value to pass to set_prefix()...
> 
> We should probably fix that... it's kind of bizarre.

That should change. See also 7dc6e331b87a963999e34c029e49417ff422c863 / th/bgo735512_route_metric



> > I think, we should deliberately check for is_valid(). For example,
> > construct_ip4_items() iterates over addresses and prints
> > nm_ip_address_get_address (addr) without checking that an address is set
> > (OK, arguably glib printf is save against printing a NULL string).
> 
> nm_utils_ip4_addresses_from_variant() will only return valid addresses



> 
> > nmtst_assert_ip4_address_equals() can still be useful at other places
> 
> It *could* be useful, but we're not actually using it there. I think it's
> better to remove it now that it's unused, and if someone wants to use it in
> tests again later, they can add it back.

Utility functions are often _not_ added/used when they are not available. The verbosity of some test cases shows that. I think it should stay.



> > +    g_return_val_if_fail (addr != NULL, NULL);
> >               ^^^
> 
> For some reason, gcc never gives me warnings about that... it must be because
> of -O0 I guess.

ah. I mostly compile with clang. But it's good if we use different compilers.





I piggyback another commit on your branch.
>> libnm: fix assigning output parameter nm_utils_ip6_addresses_from_variant()

> > »···if (!*ip) {
> > »···»···g_return_val_if_fail (null_any, FALSE);
> > »···»···return TRUE;
> > »···}
> > 
> > I think, canonicalize_ip() should not g_return depending on the argument it is
> > supposed to check.
> 
> It's a programmer error to pass NULL unless null_any is set. All of the
> original callers tested for that beforehand, but the ones added in "libnm-core:
> extract NMSettingIPConfig superclass out of IP4, IP6 classes" didn't.

all callers of canonicalize_ip() (that pass @null_any FALSE) do something like:

»···valid_addr = canonicalize_ip (address->family, &addr, FALSE);
»···g_return_if_fail (valid_addr);

hence, the user now gets two assertion failures. I think that is wrong.




> > »···struct in6_addr addr_buf = IN6ADDR_ANY_INIT;
> > 
> > doesn't need initialization.
> 
> as mentioned above, the memcmp later on depends on it having been initialized

I don't think so. addr_buf will be initialized by:

+    if (inet_pton (family, *ip, &addr_buf) != 1)
+         return FALSE;



> > I dislike the static buffer in canonicalize_ip().
> 
> It is simple and causes no problems in practice.
> 
> > how about tightening up NMIPAddress, for example, we don't need so many bits
> > for family or prefix.
> > 
> > We can even pack it more using bitfields:
> 
> That micro-optimizes memory usage at the expense of micro-UN-optimizing access
> speed (since bitfield operations are going to be slower than word-sized
> operations).

sure it's slower. It's not optimizing speed :)



> > -    int prefix, family;
> > +    guint8 prefix;
> > +    char *address, *gateway;
> 
> Note that that change wouldn't actually make the struct smaller, since the
> compiler will add padding to align address to an 8-byte boundary anyway.

together with the bitfield, it indeed doesn't matter whether prefix is guint8 or guint32. But what's the advantage of int prefix over gint8? Speed?

 struct NMIPAddress {
+    guint8 family;
+    guint8 prefix;
     guint refcount;
     char *address;
-    int prefix, family;
     GHashTable *attributes;
 };

this saves an int... why not?



> > route metric should be guint32 type.
> 
> fixed, though I'm still pondering the int64 thing

ah right. I forgot that NMIPRoute is also used for NMSettingIPxConfig. Indeed, can we make this a gint64, with range [-1,G_MAXUINT32] ?


> > todo: could we implement the lists as GPtrArray?
> 
> Yes. It would probably simplify things given that the externally-visible
> properties are already ptrarrays. But that's all internal and can happen at any
> time.

"any time" sounds a bit like "never" :) Can you add a FIXME comment?


> > maybe it would be useful to have wrappers to pass get/set addresses:
> >   nm_ip_address_set_address (addr, nm_utils_inet4_ntop (addr_array[0], NULL));
> > to
> >   nm_ip_address_set_address_ipv4 (addr, addr_array[0]);
> > (which would save canonicalize_ip() call).
> 
> I pushed a commit "[WIP] add nm_ip_address_set_binary(), etc", but I'm not sure
> it's worth it, since it only gets used in a few places (which was part of the
> point of changing from binary to strings in the first place; in most places,
> the string representation is more convenient).

I do like it.


> > Also in nm_utils_ip4_addresses_from_variant(), is_valid() would be useful to
> > skip invalid addresses. We already don't trust the other side fully and check:
> >   if (length < 3) {
> > but we don't check for invalid prefix.
> 
> mmm... ok, need to do something here still

you mean, later?


> > +    addr = nm_ip_address_new (AF_INET);
> > +    nm_ip_address_set_address (addr, "2.2.2.2");
> > +    nm_ip_address_set_prefix (addr, 24);
> > 
> > how about adding nmtst_ip_address_create?
> 
> So... in an earlier version of this branch, I had made NMIPAddress and
> NMIPRoute immutable, and you had to pass all of the arguments at construct
> time. I eventually gave up on that, because there are places where the
> mutability is just very useful. (In particular,
> clients/tui/nm-editor-bindings.c). And then I tried having it where
> nm_ip_address_new() still took the address/prefix arguments, but you could also
> use setters later if you preferred. But then it was weird that in some cases
> you'd be passing dummy values to the new() function
> 
> So... yes, we could add an nmtst function, but maybe we should just add an
> NMIPAddress function instead? Not sure about the name...
> nm_ip_address_new_full() maybe?

Oh, I really really like immutable, refcounted objects. Yeah, in some cases that makes it more inconvenient to use, but I think it makes a better API.

E.g. suppose we would add is_valid() and nm_setting_ip_config_add_address() would only allow valid addresses. Having mutable objects, that check is not useful, because the user can modify the NMIPAddress later.

(reviewing the squashed branch)

> libnm-core,all: merge IPv4 and IPv6 address/route types

In nm-setting-ip4-config/ip6-config verify(), is the inet_pton() really necessary?  Wouldn't the address have already been verified at creation/set time by nm_ip_address_set_address()?  I think all we need to check for here is NULL?  (since the address could have been created but nm_ip_address_set_address() never called on it).  If we don't already (and I think we do) we should ensure that an NMIpAddress can never be created with invalid attributes.

But later in the patch too (nm-utils.h, nm_utils_ip4_addresses_to_variant()) there's an inet_pton(), and what guarantee do we have that the string we're passing is !NULL?

"else" style nit in ifcfg-rh/reader.c in read_full_ip4_address():

+		g_free (value);
+	}
 	else {
 		gboolean read_success;

Also here, the "Validate the prefix" bits are dead code, because nm_ip_address_set_prefix() already enforces the right prefix maximums, right?  Same for the routing prefix validation bits.  If the prefix is out-of-bounds it simply won't be set so we should instead be checking "if (!nm_ip_address_get_prefix())" I think.

In read_route_file_legacy() you rewrote the GRegex stuff so that it no longer frees the return value of g_match_info_fetch(), but that function returns an allocated value so we still need to free it, both in the 'error:' case and when the loop continues.  Same for read_route6_file().

For keyfile/writer.c, in write_ip_values(), would it make a bit more sense to pass the setting GType instead of doing a strcmp() on the name?

Also "* The current version support reading of the above form." might as well fix the "support" -> "supports" while we're here.

> libnm-core: add NMIPAddress/NMIPRoute attributes, use for labels

Should we try to add some type information to the values too?  This is adding about the same sort of thing as we did for the bond options which eventually required a bunch of GParamSpec-like metadata (eg, get-option-by-name, validate, get list of valid options, defaults, etc).  Should we do the same here?  I think we found it useful in the clients so we didn't have to duplicate known options and defaults and stuff (so we don't make the same mistake the kernel does).  If we do this, perhaps a separate private helper struct to contain all this functionality (say NMAttributes) so we could use the same code from Bond and the IP stuff.  (----> though I see later this would be a PITA for the D-Bus side with AddressData/RouteData so maybe we just suck this up for now)

nm_ip_address/route_get_attribute() and get_attribute_names() aren't exported in libnm.ver?

> libnm-core: extract NMSettingIPConfig superclass out of IP4, IP6 classes

Should it be NMSettingIpConfig?  We moved most of the rest of the classes to full studly-caps (Vpn, Adsl, Wimax, Ppp, Pppoe) so should this follow suit?  Though to be consistent with IP4/IP6 we could keep it IP, or we could rename all of them.  Not sure I care much either way.

NM_SETTING_IP_CONFIG_DHCP_SEND_HOSTNAME should really be part of the base class since it applies anywhere _DHCP_HOSTNAME would.  An oversight in the current codebase but we should correct it here.

nm_connection_get_setting_ip4_config() - is the introspection return type correct here, or is that just a hint to bindings what type is actually returned even though the function's return is different than the introspection?  The docs just read a bit odd here versus the actual return type. (same for IP6)

The nm_ip_address_dup() g_return_val_if_fail() fixes should be part of "libnm-core, all: merge IPv4 and IPv6 address/route types".

I think we should validate that the route/address family matches the setting family in add_route() and add_address() too.  That pushes the warning closer to the source.

"* FIXME SHOULDN'T WE DEPRECATE dhcp-send-hostname?" -> we shouldn't since it has use beyond just 'dhcp-hostname'.  If "dhcp-hostname" == NULL but "dhcp-send-hostname" == TRUE, then NM will send the machine hostname to the server.  So dhcp-send-hostname has a use beyond just "dhcp-hostname" itself and we should fix the property comments to reflect that.  "dhcp-send-hostname" is the generic switch for whether to send the hostname or not, and the hostname source is either the setting or the machine name.  (as above we should also move the SEND_HOSTNAME property to NMSettingIPConfig too...)

src/nm-ip4-config.c, ifupdown/parser.c, test-keyfile.c, 

s_ip4 = NM_SETTING_IP_CONFIG (nm_setting_ip4_config_new ());

No need to cast this anymore since nm_setting_ip4_config_new() returns NMSettingIPConfig now.

nm_ipX_config_merge_setting() should validate the family of the setting they get passed to them.

> libnm-core: add NMSettingIPConfig:gateway, drop NMIPAddress:gateway

At this point, since the NMIPAddress doesn't contain much other than what GInetAddress contains, could we just make NMIPAddress a subclass and implement the attributes on top?  Or would that be too heavy since it's a GObject, and given that we use address->XXX everywhere and would have to convert to accessors?

'make check' failure in libnm-core/tests/test-general.c:

/core/general/test_setting_ip4_gateway: OK
/core/general/test_setting_ip6_gateway: **
ERROR:test-general.c:3547:test_setting_ip6_gateway: assertion failed (nm_setting_ip_config_get_gateway (s_ip6) == "abcd::1"): (NULL == "abcd::1")
/bin/sh: line 5: 27821 Aborted                 ${dir}$tst
FAIL: test-general

keyfile/reader.c:  what's up with setting the gateway as an attribute on the IP address?  I feel like there should be a better way to pass the legacy gateway back...  Also for keyfile, maybe we should just accept the legacy format when reading and rewrite it when saving with the new separate 'gateway' key.  Seems less confusing going forward.

Should we:

#define NM_IP_ADDRESS_VARIANT_TYPE       "sua{ss}"
#define NM_IP_ADDRESS_ARRAY_VARIANT_TYPE "a(" NM_IP_ADDRESS_VARIANT_TYPE ")"
#define NM_IP_ROUTE_VARIANT_TYPE         "susua{ss}"
#define NM_IP_ROUTE_ARRAY_VARIANT_TYPE   "a(" NM_IP_ROUTE_VARIANT_TYPE ")"

somewhere in nm-utils-private.h?  They get used quite a few times and it might help against inadvertent typos to use #defines.

> core: add some additional AddressData to NMIP4Config/NMIP6Config

If we're exposing 'lifetime' then we should also expose some time base from when the lifetime started ('timestamp') otherwise clients won't be able to do much with this.  But then those are still arbitrary numbers and clients have no idea what units 'timestamp' is in or how to relate that to the system clock.  We've run into this issue with cyphermox's WiFi LastSeen patches too, becuase clients may want to know how old an AP is but there's easy way to tell them due to this timestamp thing.  Thoughts?

> libnm: create NMIPConfig as parent of NMIP4Config and NMIP6Config

Should there be some way to retrieve the family of the NMIPConfig?  Now that NMIP4Config/NMIP6Config are private classes, there seems to be no way to get that information since we can't use NM_IS_IP4_CONFIG() anymore.

Should NMIPConfig be an abstract type?

> libnm: create NMDhcpConfig as parent of NMDhcp4Config and NMDhcp6Config

Same comments as for IPConfig...

> [WIP] add nm_ip_address_set_binary(), etc

My feeling is "yes" but I don't have concrete cases for that yet.

> libnm-core: don't serialize empty address-labels

The commit message is a double-negative, maybe "If no addresses have labels, then..."

(In reply to comment #31)
> all callers of canonicalize_ip() (that pass @null_any FALSE) do something like:
> 
> »···valid_addr = canonicalize_ip (address->family, &addr, FALSE);
> »···g_return_if_fail (valid_addr);
> 
> hence, the user now gets two assertion failures. I think that is wrong.

No, because nm_ip_address_set_address() will never call canonicalize_ip with a NULL addr:

	g_return_if_fail (addr != NULL);

	valid_addr = canonicalize_ip (address->family, &addr, FALSE);
	g_return_if_fail (valid_addr);

Each function checks its own preconditions. nm_ip_address_set_address()'s precondition is that addr isn't NULL, so it checks that, to make sure external code that calls nm_ip_address_set_address() isn't buggy. canonicalize_ip()'s precondition is that *ip is only NULL if null_any is TRUE, so it checks that, to make sure that nm-setting-ip-config.c-internal code isn't buggy.

> > > »···struct in6_addr addr_buf = IN6ADDR_ANY_INIT;
> > > 
> > > doesn't need initialization.
> > 
> > as mentioned above, the memcmp later on depends on it having been initialized
> 
> I don't think so. addr_buf will be initialized by:
> 
> +    if (inet_pton (family, *ip, &addr_buf) != 1)
> +         return FALSE;

If family is AF_INET then inet_pton() will only modify the first four bytes of addr_buf, but later, we always memcmp() the entire buffer, so we don't need separate AF_INET and AF_INET6 cases. So addr_buf needs to be initialized to all-0s in the IPv4 case for the memcmp() to work correctly.

> But what's the advantage of int prefix over gint8? Speed?

Normalcy. We don't use gint8 for prefix anywhere else either. And yes, "int" is more or less defined to be "whatever size integer the processor handles most easily".

> > > Also in nm_utils_ip4_addresses_from_variant(), is_valid() would be useful to
> > > skip invalid addresses. We already don't trust the other side fully and check:
> > >   if (length < 3) {
> > > but we don't check for invalid prefix.
> > 
> > mmm... ok, need to do something here still
> 
> you mean, later?

No, before merging. There are still a handful of issues to be resolved with this branch, including that, the gint64 metric thing, the binary address-setting methods (which it sounds like both you and dcbw want to keep, so I guess that's done now), and the stuff related to send-dhcp-hostname.

> Oh, I really really like immutable, refcounted objects. Yeah, in some cases
> that makes it more inconvenient to use, but I think it makes a better API.

One big issue with having them be fully immutable is that then you need to pass all attributes at construct time too...

Perhaps we could make them become "sealed" once they're added to a Setting or Config object though?

(In reply to comment #32)
> > libnm-core,all: merge IPv4 and IPv6 address/route types
> 
> In nm-setting-ip4-config/ip6-config verify(), is the inet_pton() really
> necessary?

No... this gets fixed later when NMSettingIPConfig gets split out. I'll copy the fix back to here.

> If we don't already (and I
> think we do) we should ensure that an NMIpAddress can never be created with
> invalid attributes.

Well, it has a NULL address when it's first created...

Maybe we should make the constructor be

  NMIPAddress *nm_ip_address_new (int family, const char *address, int prefix);

...

Actually, it can take a GError too, and then that saves us a lot of redundant error checking code elsewhere...

> Also here, the "Validate the prefix" bits are dead code, because
> nm_ip_address_set_prefix() already enforces the right prefix maximums, right? 
> Same for the routing prefix validation bits.  If the prefix is out-of-bounds it
> simply won't be set so we should instead be checking "if
> (!nm_ip_address_get_prefix())" I think.

Well, if it's out of bounds, we don't want to call nm_ip_address_set_prefix(), because that will g_return_if_fail(). But it's dumb to have to independently verify the properties in every backend, so either we should do the constructor-with-GError as above, or make set_prefix(), etc, take a GError.

> > libnm-core: add NMIPAddress/NMIPRoute attributes, use for labels
> 
> Should we try to add some type information to the values too?  This is adding
> about the same sort of thing as we did for the bond options which eventually
> required a bunch of GParamSpec-like metadata

I have two contradictory answers for that

  1. The bond options are the core data in an NMSettingBond, so you need
     the metadata for them, whereas the address/route attributes are more
     for "stuff that isn't important enough to get its own API". And there
     probably won't ever be as many of them as there are bond options.

  2. We could possibly add API to NMIPAddress/NMIPRoute for specific
     important attributes in the future, and hide the fact that they
     are implemented as attributes underneath...

Maybe I should make the attributes be an a{sv} rather than an a{ss}? That gives us a bit more metadata (which I guess could be useful with the attributes added later to NMIPConfig).

> > libnm-core: extract NMSettingIPConfig superclass out of IP4, IP6 classes
> 
> Should it be NMSettingIpConfig?  We moved most of the rest of the classes to
> full studly-caps (Vpn, Adsl, Wimax, Ppp, Pppoe) so should this follow suit?

The convention in glib and gtk, for whatever reason, is that two-character abbreviations (UI, IP, IO, etc) have both letter capitalized, but longer ones are initial caps only, so that's what I went with when I renamed things. (Except that I only did it one direction, so "NMVpnPluginUiInterface" is now wrong, which I may still fix.)

> NM_SETTING_IP_CONFIG_DHCP_SEND_HOSTNAME should really be part of the base class
> since it applies anywhere _DHCP_HOSTNAME would.  An oversight in the current
> codebase but we should correct it here.

OK, I was assuming that it was intentional that IP4 had that property, but IP6 did not.

> nm_connection_get_setting_ip4_config() - is the introspection return type
> correct here, or is that just a hint to bindings what type is actually returned
> even though the function's return is different than the introspection?

Correct. Python and JavaScript might not care much, but C# and Java need it so they know (at compile time) that connection.GetSettingIP6Config().ip6_privacy is OK.

> > libnm-core: add NMSettingIPConfig:gateway, drop NMIPAddress:gateway
> 
> At this point, since the NMIPAddress doesn't contain much other than what
> GInetAddress contains, could we just make NMIPAddress a subclass and implement
> the attributes on top?  Or would that be too heavy since it's a GObject, and
> given that we use address->XXX everywhere and would have to convert to
> accessors?

Well, we could do:

  struct _NMIPAddress {
      GInetAddress parent;

      int family;
      char *address;
      int prefix;
  };

if we wanted....

But it doesn't seem like there'd be much advantage in making it a subclass of GInetAddress...

> keyfile/reader.c:  what's up with setting the gateway as an attribute on the IP
> address?  I feel like there should be a better way to pass the legacy gateway
> back...

Yeah... originally I was actually using "gateway" attributes that way in all the backends, to preserve the extra gateway values even though they never got used. But then I decided that was pointless. I guess I just left that code as it was because it's simple. But I can fix it.

> Also for keyfile, maybe we should just accept the legacy format when
> reading and rewrite it when saving with the new separate 'gateway' key.  Seems
> less confusing going forward.

There are other places where we intentionally write data in backward-compatible ways rather than the preferred ways. Eg, we accept an empty string for a route's next-hop in reader.c, but we always write "0.0.0.0" or "::" in writer.c, for backward-compatibility. So I did the same thing here. But I'm happy to change it if you want...

> Should we:
> 
> #define NM_IP_ADDRESS_VARIANT_TYPE       "sua{ss}"
> #define NM_IP_ADDRESS_ARRAY_VARIANT_TYPE "a(" NM_IP_ADDRESS_VARIANT_TYPE ")"
> #define NM_IP_ROUTE_VARIANT_TYPE         "susua{ss}"
> #define NM_IP_ROUTE_ARRAY_VARIANT_TYPE   "a(" NM_IP_ROUTE_VARIANT_TYPE ")"
> 
> somewhere in nm-utils-private.h?  They get used quite a few times and it might
> help against inadvertent typos to use #defines.

Yeah, probably I guess. One issue though is that we don't always use the string in exactly that format. Eg, in some places we need "(&su&sua{ss})". (It's unfortunate that GVariant format strings work that way...)

> > core: add some additional AddressData to NMIP4Config/NMIP6Config
> 
> If we're exposing 'lifetime' then we should also expose some time base from
> when the lifetime started ('timestamp')....  Thoughts?

I guess it would make more sense to have it be a unix time value indicating when the address expires?

But also, this patch was kind of just a proof of concept, and maybe we don't actually care about exposing this information?

> > libnm: create NMIPConfig as parent of NMIP4Config and NMIP6Config
> 
> Should there be some way to retrieve the family of the NMIPConfig?

Probably makes sense.

> Now that NMIP4Config/NMIP6Config are private classes

which was a last-minute change, btw... I think it makes sense though?

> Should NMIPConfig be an abstract type?

Yup

> squash! libnm-core: add NMSettingIPConfig:gateway, drop NMIPAddress:gateway

I have pushed a fixup.

We should add/move docs from libnm-util to libnm-core at some point. They diverged and ,for example, description for 'gateway' is missing now, 'addresses' are obsolete.

(In reply to comment #32)
> For keyfile/writer.c, in write_ip_values(), would it make a bit more sense to
> pass the setting GType instead of doing a strcmp() on the name?

Nope, because we need the setting name itself to pass to nm_keyfile_plugin_kf_set_string() anyway

> NM_SETTING_IP_CONFIG_DHCP_SEND_HOSTNAME should really be part of the base class
> since it applies anywhere _DHCP_HOSTNAME would.  An oversight in the current
> codebase but we should correct it here.

I fixed that in a fixup, and then added another commit:

    core, clients: implement dhcp-send-hostname for IPv6
    
This is possibly problematic since it means we will now send the persistent hostname with DHCPv6 requests when ip6.dhcp-hostname is unset, whereas before we sent nothing. And there is no way to disable this property in ifcfg-rh. OTOH, there's no way to disable this (or to set dhcp-hostname) from nm-connection-editor anyway, so maybe this is just not a big deal...

> s_ip4 = NM_SETTING_IP_CONFIG (nm_setting_ip4_config_new ());
> 
> No need to cast this anymore since nm_setting_ip4_config_new() returns
> NMSettingIPConfig now.

No, nm_connection_get_setting_ip4_config() returns NMSettingIPConfig, but all the nm_setting_*_new() methods return NMSetting*.

> 'make check' failure in libnm-core/tests/test-general.c:

Not seeing that

> Should we:
> 
> #define NM_IP_ADDRESS_VARIANT_TYPE       "sua{ss}"

I ended up not doing this; we don't #define most of the other existing complicated strings like "a(ayuay)" either... Can be added later.



re-pushed (to danw/addressdata-bgo682946 again, not addressdata-squashed), with fixups to show the main changes made since the last review. The new non-fixup commits are:

0f99645 core, clients: implement dhcp-send-hostname for IPv6
42cd13f all: allow route metrics to be "0"

> Maybe I should make the attributes be an a{sv} rather than an a{ss}? That gives
> us a bit more metadata (which I guess could be useful with the attributes added
> later to NMIPConfig).

btw, also still interested in an answer to this...

(And if we were going to do that, then should we make the AddressData and RouteData be just aa{sv} rather than a(sua{sv}) / a(susxa{sv})? ie, *everything* is an attribute?)

(In reply to comment #34)
> > If we don't already (and I
> > think we do) we should ensure that an NMIpAddress can never be created with
> > invalid attributes.
> 
> Well, it has a NULL address when it's first created...
> 
> Maybe we should make the constructor be
> 
>   NMIPAddress *nm_ip_address_new (int family, const char *address, int prefix);
> 
> ...
> 
> Actually, it can take a GError too, and then that saves us a lot of redundant
> error checking code elsewhere...

Yeah, this sounds better.

> > Also here, the "Validate the prefix" bits are dead code, because
> > nm_ip_address_set_prefix() already enforces the right prefix maximums, right? 
> > Same for the routing prefix validation bits.  If the prefix is out-of-bounds it
> > simply won't be set so we should instead be checking "if
> > (!nm_ip_address_get_prefix())" I think.
> 
> Well, if it's out of bounds, we don't want to call nm_ip_address_set_prefix(),
> because that will g_return_if_fail(). But it's dumb to have to independently
> verify the properties in every backend, so either we should do the
> constructor-with-GError as above, or make set_prefix(), etc, take a GError.

I vote constructor-with-GError.

> > > libnm-core: add NMIPAddress/NMIPRoute attributes, use for labels
> > 
> > Should we try to add some type information to the values too?  This is adding
> > about the same sort of thing as we did for the bond options which eventually
> > required a bunch of GParamSpec-like metadata
> 
> I have two contradictory answers for that
> 
>   1. The bond options are the core data in an NMSettingBond, so you need
>      the metadata for them, whereas the address/route attributes are more
>      for "stuff that isn't important enough to get its own API". And there
>      probably won't ever be as many of them as there are bond options.
> 
>   2. We could possibly add API to NMIPAddress/NMIPRoute for specific
>      important attributes in the future, and hide the fact that they
>      are implemented as attributes underneath...

Ok.

> Maybe I should make the attributes be an a{sv} rather than an a{ss}? That gives
> us a bit more metadata (which I guess could be useful with the attributes added
> later to NMIPConfig).

Sure, that sounds fine.

> > > libnm-core: extract NMSettingIPConfig superclass out of IP4, IP6 classes
> > 
> > Should it be NMSettingIpConfig?  We moved most of the rest of the classes to
> > full studly-caps (Vpn, Adsl, Wimax, Ppp, Pppoe) so should this follow suit?
> 
> The convention in glib and gtk, for whatever reason, is that two-character
> abbreviations (UI, IP, IO, etc) have both letter capitalized, but longer ones
> are initial caps only, so that's what I went with when I renamed things.
> (Except that I only did it one direction, so "NMVpnPluginUiInterface" is now
> wrong, which I may still fix.)

Ok, just wondering.  I'm fine with how it is now.

> > NM_SETTING_IP_CONFIG_DHCP_SEND_HOSTNAME should really be part of the base class
> > since it applies anywhere _DHCP_HOSTNAME would.  An oversight in the current
> > codebase but we should correct it here.
> 
> OK, I was assuming that it was intentional that IP4 had that property, but IP6
> did not.

Nope, I think we just never added it for some reason.  It should apply to both.

> > > libnm-core: add NMSettingIPConfig:gateway, drop NMIPAddress:gateway
> > 
> > At this point, since the NMIPAddress doesn't contain much other than what
> > GInetAddress contains, could we just make NMIPAddress a subclass and implement
> > the attributes on top?  Or would that be too heavy since it's a GObject, and
> > given that we use address->XXX everywhere and would have to convert to
> > accessors?
> 
> Well, we could do:
> 
>   struct _NMIPAddress {
>       GInetAddress parent;
> 
>       int family;
>       char *address;
>       int prefix;
>   };
> 
> if we wanted....
> 
> But it doesn't seem like there'd be much advantage in making it a subclass of
> GInetAddress...

Ok, I'm fine with leaving as-is.

> > keyfile/reader.c:  what's up with setting the gateway as an attribute on the IP
> > address?  I feel like there should be a better way to pass the legacy gateway
> > back...
> 
> Yeah... originally I was actually using "gateway" attributes that way in all
> the backends, to preserve the extra gateway values even though they never got
> used. But then I decided that was pointless. I guess I just left that code as
> it was because it's simple. But I can fix it.

If it's not too much trouble, fixing that would be nice.

> > Also for keyfile, maybe we should just accept the legacy format when
> > reading and rewrite it when saving with the new separate 'gateway' key.  Seems
> > less confusing going forward.
> 
> There are other places where we intentionally write data in backward-compatible
> ways rather than the preferred ways. Eg, we accept an empty string for a
> route's next-hop in reader.c, but we always write "0.0.0.0" or "::" in
> writer.c, for backward-compatibility. So I did the same thing here. But I'm
> happy to change it if you want...

Sure, and then if GATEWAY itself exists we just ignore any 0...G_MAXUINT32 gateways?

> > Should we:
> > 
> > #define NM_IP_ADDRESS_VARIANT_TYPE       "sua{ss}"
> > #define NM_IP_ADDRESS_ARRAY_VARIANT_TYPE "a(" NM_IP_ADDRESS_VARIANT_TYPE ")"
> > #define NM_IP_ROUTE_VARIANT_TYPE         "susua{ss}"
> > #define NM_IP_ROUTE_ARRAY_VARIANT_TYPE   "a(" NM_IP_ROUTE_VARIANT_TYPE ")"
> > 
> > somewhere in nm-utils-private.h?  They get used quite a few times and it might
> > help against inadvertent typos to use #defines.
> 
> Yeah, probably I guess. One issue though is that we don't always use the string
> in exactly that format. Eg, in some places we need "(&su&sua{ss})". (It's
> unfortunate that GVariant format strings work that way...)

Yeah, though I looked during my review to make sure and I think that only happens in one place.  Still worth doing I think.

> > > core: add some additional AddressData to NMIP4Config/NMIP6Config
> > 
> > If we're exposing 'lifetime' then we should also expose some time base from
> > when the lifetime started ('timestamp')....  Thoughts?
> 
> I guess it would make more sense to have it be a unix time value indicating
> when the address expires?
> 
> But also, this patch was kind of just a proof of concept, and maybe we don't
> actually care about exposing this information?

Yeah, maybe lets not expose it yet until we now how to do the timestamp stuff.  The problem with unix time is clock reset/daylight-savings and suspend/resume; need to figure that stuff out too.

> > > libnm: create NMIPConfig as parent of NMIP4Config and NMIP6Config
> > 
> > Should there be some way to retrieve the family of the NMIPConfig?
> 
> Probably makes sense.
> 
> > Now that NMIP4Config/NMIP6Config are private classes
> 
> which was a last-minute change, btw... I think it makes sense though?

Yeah, I think it makes sense.

> > Should NMIPConfig be an abstract type?
> 
> Yup

Cool.

(In reply to comment #37)
> > Maybe I should make the attributes be an a{sv} rather than an a{ss}? That gives
> > us a bit more metadata (which I guess could be useful with the attributes added
> > later to NMIPConfig).
> 
> btw, also still interested in an answer to this...
> 
> (And if we were going to do that, then should we make the AddressData and
> RouteData be just aa{sv} rather than a(sua{sv}) / a(susxa{sv})? ie,
> *everything* is an attribute?)

Sure, why not?  Would probably make marshalling a lot easier, and since everything uses accessors it would be an internal-only change.  I guess validation would be a bit more work since you have to ensure that specific values are always present, but that's not a lot of code.

> fixup! (move dhcp-send-hostname from IP4 to IP)

The gtkdoc for dhcp-send-hostname still has a '4' in it:

* NMSettingIP4Config:dhcp-send-hostname:

> fixup! (add nm_ip_config_get_family())

PROP_FAMILY is an 'int' type?  Even though address family is usually an int in other code, I think this should be 'uint' since AF_UNSPEC is 0 and that's the "undefined" value.

> fixup! (add nm_dhcp_config_get_family())

Same thing here; uint perhaps?

void
nm_ip_route_unref (NMIPRoute *route)
{
»···g_return_if_fail (route != NULL);


can we make the unref functions silently accept NULL?
Like g_free and contrary to g_object_unref.






>> libnm-core, all: merge IPv4 and IPv6 address/route types

+    /* We don't accept default routes as NetworkManager handles it itself */
+    if (!strcmp (dest, "0.0.0.0") || !strcmp (dest, "::")) {

this would not catch "0.0.0.00" for example. This check should at the end of nmc_parse_and_build_route() after @dest is normalized.

nm_ip_route_new() and all the setters now perform basic validation, hence you cannot create an invalid object anymore.
This basic validation only validates each property by itself, not together. I still think that is not enough.  For example routes with a non-zero host-part in the destination (e.g. 192.168.0.0/8) should be invalid.


I know, you already dismissed the idea of sealed objects. But how great would be the following:

- add a nm_ip_route_is_valid(route, flags, error) function. It checks validity considering more then one property. The flags could be like NO_DEFAULT set by nm_setting_ip_config_add_route(), while in other cases it might be alright.

- make the class sealable:
    gboolean nm_ip_route_seal (route, flags, error)
  sealing performs validation (and returns an error if validation fails).

  nm_setting_ip_config_add_route() could do:

  if (!nm_ip_route_seal (route, NO_DEFAULT, NULL))
      g_return_val_if_reached (FALSE);

- g_ptr_array_add (priv->routes, nm_ip_route_dup (route));
+ g_ptr_array_add (priv->routes, nm_ip_route_ref (route));

OK, squashed all the existing fixups, and mostly squashed Thomas's except for:

>-				metric = 1;
>+				metric = -1;

in ifnet/connection_parser.c. Yes, the existing code makes no sense, but it's someone else's nonsense, not ours, so I'm leaving it as it was.


(In reply to comment #38)
> > There are other places where we intentionally write data in backward-compatible
> > ways rather than the preferred ways. Eg, we accept an empty string for a
> > route's next-hop in reader.c, but we always write "0.0.0.0" or "::" in
> > writer.c, for backward-compatibility. So I did the same thing here. But I'm
> > happy to change it if you want...
> 
> Sure, and then if GATEWAY itself exists we just ignore any 0...G_MAXUINT32
> gateways?

This was talking about keyfile, not ifcfg-rh... (In ifcfg-rh, the behavior is that we now always write GATEWAY, not GATEWAY#n, which is both the most correct and the most backward-compatible behavior.)

Anyway, if we're going to break backward-compatibility in keyfile we should drop the other backward-compatibility constraints too, so I think that should happen separately.

> > > Should we:
> > > 
> > > #define NM_IP_ADDRESS_VARIANT_TYPE       "sua{ss}"

Now that now we're switching to "aa{sv}", this doesn't make as much sense. (At any rate, it would be strictly internal and could happen later.)

> Yeah, maybe lets not expose [lifetime / expiry] yet until we know how to do the timestamp stuff. 

OK, I removed the entire "additional attributes" patch ("peer-address", "expires", and ipv6 "ifa-flags"), because I realized that if we're going to have additional metadata there, then we probably want to signal PropertiesChanged when the additional metadata changes too, right? So there's a little bit more work to be done there, and it can happen later, since that's additions, not changes.


(In reply to comment #39)
> > (And if we were going to do that, then should we make the AddressData and
> > RouteData be just aa{sv} rather than a(sua{sv}) / a(susxa{sv})? ie,
> > *everything* is an attribute?)
> 
> Sure, why not?

Pushed three fixups, one to "libnm-core: add NMIPAddress/NMIPRoute attributes, use for labels", changing attributes to GVariants, and two to "libnm-core, libnm, core: add AddressData and RouteData properties", first updating it to the new attribute API without changing functionality, then changing the marshalling to "aa{sv}". (There are also changes to the "allow metric to be 0" commit, but that's just merged into the existing commit because it didn't make sense to fix the merge conflicts there without also fixing the logic.)

Unanswered: should nm_ip_address_get_attribute(addr, "address") return the address? (Currently it does not.)

(In reply to comment #40)
> > fixup! (add nm_ip_config_get_family())
> 
> PROP_FAMILY is an 'int' type?  Even though address family is usually an int in
> other code, I think this should be 'uint' since AF_UNSPEC is 0 and that's the
> "undefined" value.

Hm... I feel like it's better to stay consistent with the other code where family is an 'int'...

(In reply to comment #41)
> can we make the unref functions silently accept NULL?
> Like g_free and contrary to g_object_unref.

Looking through the code, it doesn't seem like that functionality would be used all that much.

> this would not catch "0.0.0.00" for example. This check should at the end of
> nmc_parse_and_build_route() after @dest is normalized.

fixup'ed

> This basic validation only validates each property by itself, not together. I
> still think that is not enough.  For example routes with a non-zero host-part
> in the destination (e.g. 192.168.0.0/8) should be invalid.

I started poking at this... it's easy enough to enforce that at route-creation time, but you can't enforce it in either set_dest() or set_prefix(), since the caller could change those in either order.

So no change here.


repushed with the above changes

> Unanswered: should nm_ip_address_get_attribute(addr, "address") return the
address? (Currently it does not.)

I say "not now".


> fixup! (change address/route attributes to GVariant)

'value' isn't used at all in nm_ip_address_get_attribute_names() so we can just pass NULL to g_hash_table_iter_next().  Same for route_get_attribute_names().

Might make sense to add nm_ip_XXXX_set_attribute_from_string() later too.

> fixup! (make AddressData and RouteData aa{sv})

nm_utils_ip_addresses_from_variant() - doesn't the ""Ignoring invalid address: %s", error->message" condition need a 'continue'?

---

The rest of it looks fine and anything I haven't directly responded to here is OK with me as-is.

make[2]: Leaving directory `/home/dcbw/Development/fdo/NetworkManager/NetworkManager-0.9.11.0/_build/vapi'
rm -f config.status config.cache config.log configure.lineno config.status.lineno
rm -f Makefile
ERROR: files left in build directory after distclean:
./src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-Test_Write_Wired_Static_IP6_Only_With_Gateway_::ffff:255.255.255.255
./src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-Test_Write_Wired_Static_IP6_Only_With_Gateway_2001:db8:8:4::2
./src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-Test_Write_Wired_Static_IP6_Only_With_Gateway_NULL
./src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-Test_Write_Wired_Static_IP6_Only_With_Gateway_::
make[1]: *** [distcleancheck] Error 1
make[1]: Leaving directory `/home/dcbw/Development/fdo/NetworkManager/NetworkManager-0.9.11.0/_build'
make: *** [distcheck] Error 1

test_write_wired_static_ip6_only_gw() has a commented-out unlink that shouldn't be commented:

	//unlink (testfile);

fixed and pushed