Re: [PATCH] device: allow for applying IPv4 and IPv6 settings in parallel

[Dan Williams <dcbw redhat com>]

On Mon, 2011-07-25 at 18:12 -0400, Mathieu Trudel-Lapierre wrote:
> Hi,
> 
> We don't really need to wait before both IPv4 and IPv6 are established before
> applying all the settings to the device. Instead, we can apply each separately
> when they are ready, which will bring up the interface sooner.
> 
> Patch is attached; since it's relatively large.

Hah, my first impression was that it was pretty contained and small :)
Which is good.  A few comments:

1) Just return a gboolean from update_ip6_config_with_dhcp(); the
semantics of returning something that you've passed in are kinda weird,
and makes it unclear whether what's returned is new, got referenced,
etc.  Also add an "NMDeviceStateReason *reason" argument which gets
populated with the right error when things fail.

2) you don't want to call nm_device_state_changed() in
update_ip6_config_with_dhcp() since the functions that call
update_ip6_config_with_dhcp() do that depending on the return value; the
callers better know what to do when this function fails.

3) Also, I don't think we need to care about LINK_LOCAL in
update_ip6_config_with_dhcp() since with LINK_LOCAL you'll never be
running DHCP at all; it's link-local.  The only time you run DHCP are
AUTO (when the RA tells you to) or DHCP, so just skip the check for
LINK_LOCAL there.

4) also, just move merge_dhcp_config_to_master() above
update_ip6_config_with_dhcp() and kill the forward declaration, looks
kinda icky IMHO; don't worry about moving code around

5) You probably want the g_object_notify (G_OBJECT (device),
NM_DEVICE_INTERFACE_DHCP6_CONFIG); statement inside the success block
for nm_device_set_ip6_config () right after the 'dhcp6-change'
dispatcher event gets fired, otherwise the notify gets sent out even if
things fail which isnt' quite right.

Also, what's the reason for killing
nm_device_activate_schedule_stage5_ip_config_commit()?

Looking good so far, lets do a respin.

Thanks!
Dan

> ---
>  src/nm-device.c |  207 ++++++++++++++++++++++++++++--------------------------
>  1 files changed, 107 insertions(+), 100 deletions(-)
> 
> --
> Mathieu Trudel-Lapierre <mathieu-tl ubuntu com>
> Freenode: cyphermox, Jabber: mathieu tl gmail com
> 4096R/EE018C93 1967 8F7D 03A1 8F38 732E  FF82 C126 33E1 EE01 8C93
> _______________________________________________
> networkmanager-list mailing list
> networkmanager-list gnome org
> http://mail.gnome.org/mailman/listinfo/networkmanager-list