Regarding "platform: don't zero terminate the result GArray of get_all() functions", see also https://bugzilla.redhat.com/show_bug.cgi?id=907836#c24

> platform: refactor ip4_address_get_all() and ip6_address_get_all()

>+	addresses = g_array_sized_new (TRUE, FALSE, sizeof (NMPlatformIP4Address), 0);

You don't need "sized_new" if you don't know the size. Just use g_array_new() and drop the last arg.

Likewise in the next patch.


> platform: don't zero terminate the result GArray of get_all() functions

You could just merge this with the previous two... you're switching from clear=TRUE to clear=FALSE, there's no reason you can't switch zero_terminated too since apparently none of the callers care.

(Also, link_get_all() returns a zero-terminated list too)


> platform: refactor signals by combining added/changed/removed

>+#define NM_PLATFORM_LINK "link"
>+#define NM_PLATFORM_IP4_ADDRESS "ip4-address"
>+#define NM_PLATFORM_IP6_ADDRESS "ip6-address"
>+#define NM_PLATFORM_IP4_ROUTE "ip4-route"
>+#define NM_PLATFORM_IP6_ROUTE "ip6-route"

These aren't good signal names. They sound more like properties. If you want to avoid using "CHANGED", maybe "EVENT"?

>-lost_link (NMPlatform *platform, int ifindex, NMPlatformLink *info, NMPlatformReason reason, NMDeviceAdsl *device_adsl)
>+lost_link (NMPlatform *platform, int ifindex, NMPlatformLink *info, NMPlatformSignalChangeType change_type, NMPlatformReason reason, NMDeviceAdsl *device_adsl)

seems like it would make sense to rename some of the signal handlers. Eg, even though we only care about the "removed" case here, the handler itself should still probably be "link_changed" or something.

>-	NMDeviceAdslPrivate *priv = NM_DEVICE_ADSL_GET_PRIVATE (device_adsl);
>-	NMDevice *device = NM_DEVICE (device_adsl);
>+	NMDeviceAdslPrivate *priv;
>+	NMDevice *device;
>+
>+	if (change_type != NM_PLATFORM_SIGNAL_REMOVED)
>+		return;
>+
>+	priv = NM_DEVICE_ADSL_GET_PRIVATE (device_adsl);
>+	device = NM_DEVICE (device_adsl);

There's not really any need to make changes like that. This function isn't going to get called thousands of times in a row. You can afford to call GET_PRIVATE even if you don't use it.

>-	g_signal_connect (platform, NM_PLATFORM_LINK_CHANGED,
>+	g_signal_connect (platform, NM_PLATFORM_LINK,
> 	                  G_CALLBACK (link_changed_cb), dev);

(in NMDevice:constructor). Might as well make that a single line too like all the others above it.

(In reply to comment #2)

rebased and repushed.


 
> > platform: don't zero terminate the result GArray of get_all() functions
> 
> You could just merge this with the previous two... you're switching from
> clear=TRUE to clear=FALSE, there's no reason you can't switch zero_terminated
> too since apparently none of the callers care.

Didn't do this. The "clear=FALSE" change is not visible to callers.

> (Also, link_get_all() returns a zero-terminated list too)

Yes.


> > platform: refactor signals by combining added/changed/removed
> 
> >+#define NM_PLATFORM_LINK "link"
> >+#define NM_PLATFORM_IP4_ADDRESS "ip4-address"
> >+#define NM_PLATFORM_IP6_ADDRESS "ip6-address"
> >+#define NM_PLATFORM_IP4_ROUTE "ip4-route"
> >+#define NM_PLATFORM_IP6_ROUTE "ip6-route"
> 
> These aren't good signal names. They sound more like properties. If you want to
> avoid using "CHANGED", maybe "EVENT"?

Renamed to
NM_PLATFORM_SIGNAL_IP4_ADDRESS_CHANGED "ip4-address-changed"



Done to the rest.

looks good, although the comment I made about not reorganizing the declarations/assignments in nm-device-adsl.c also applies to nm-device.c:link_changed()

(In reply to comment #4)
> looks good, although the comment I made about not reorganizing the
> declarations/assignments in nm-device-adsl.c also applies to
> nm-device.c:link_changed()

Changed and pushed fixup.

Rebased to master, and added tree new commits.

core: preserve longest timestamp in NMIP[46]Config add address
platform: add nm_platform_ip_address_cmplifetime()
platform: extract the common fields of IPv4/IPv6 addresses and routes
fixup! platform: refactor signals by combining added/changed/removed


The previous commits did not change (except rebasing).

> platform: refactor signals by combining added/changed/removed

Just remove link_changed_cb_id and use g_signal_handlers_disconnect_by_func().

> platform: refactor signals by combining added/changed/removed

NM_PLATFORM_SIGNAL_* -> NM_PLATFORM_CHANGE_* ?  Seems a bit less confusing to me.

> platform: add nm_platform_ip_address_cmplifetime()

Why is timestamp part of the comparison?  That causes addresses to be sorted
non deterministically.  Also could mis-sort if lifetime was small and timestamp
was large for one address, and opposite for the other, which doesn't return
the one with longer lifetime.

> core: preserve longest timestamp in NMIP[46]Config add address

Shortlog says "timestamp" but the commit message says "lifetime".  I think
we want "lifetime" everywhere?

Prefere -> prefer

(In reply to comment #7)
> 
> > platform: refactor signals by combining added/changed/removed
> 
> NM_PLATFORM_SIGNAL_* -> NM_PLATFORM_CHANGE_* ?  Seems a bit less confusing to
> me.

I don't mind the name too much, it's just I already renamed them once.
Let's first come to a conclusion, because this rebase -i is going to kill me :)

For the record, I think the name is ok as-is.

Rebased to master and repushed (non-ff).

The original commits are mostly unchanged (I reworded the commit messages how necessary, regarding comment 7).
New commits (fixup!) are sorted after the commit the update.

New commits since last time are:
  e2ef7fe fixup! core: preserve later expiry in nm_ip[46]_config_add_address()
  5e7ecfc fixup! platform: add nm_platform_ip_address_cmp_expiry()
  a7c8651 fixup! platform: extract common fields of IPv4/IPv6 addresses and 
          routes to base struct
  d9b42c8 fixup! platform: refactor signals by combining added/changed/removed



(In reply to comment #7)
> > platform: refactor signals by combining added/changed/removed
> 
> Just remove link_changed_cb_id and use g_signal_handlers_disconnect_by_func().

done.


> > platform: refactor signals by combining added/changed/removed
> 
> NM_PLATFORM_SIGNAL_* -> NM_PLATFORM_CHANGE_* ?  Seems a bit less confusing to
> me.

Not done. First come up with the final name...


> > platform: add nm_platform_ip_address_cmplifetime()
> 
> Why is timestamp part of the comparison?  That causes addresses to be sorted
> non deterministically.  Also could mis-sort if lifetime was small and timestamp
> was large for one address, and opposite for the other, which doesn't return
> the one with longer lifetime.

Renamed to nm_platform_ip_address_cmp_expiry(). The previous name was misleading. We don't compare according to their (whole) lifetime, but according to which one will expire earlier/later. Thus, it needs to take into account the "timestamp". I think this is correct now (I changed some details, but in principle it's as before).


> > core: preserve longest timestamp in NMIP[46]Config add address
> 
> Shortlog says "timestamp" but the commit message says "lifetime".  I think
> we want "lifetime" everywhere?
> 
> Prefere -> prefer

Reworded. Note that the previous commit. Note that this change makes sense to do. Please do some proof-reasoning, what this commit does.



The final commit "WIP" can be ignored for the moment. Just to see where the journey goes.
The other commits I would like to commit earlier, if y'all agree...

Ok, next time I reread before sending... sorry for all the nonsensical ~English~...

Don't worry about the SIGNAL name change.  The branch looks good to me now, except the WIP commit obviously.

Branch merged to master as:

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


I removed the "WIP" commit and will post it on a later occasion.