Re: [PATCH] Fix manual IP config
Am Dienstag, 18. März 2008 19:48:28 schrieb Dan Williams: On Tue, 2008-03-18 at 12:46 -0600, Tambet Ingo wrote: On Tue, Mar 18, 2008 at 12:36 PM, Dan Williams [EMAIL PROTECTED] wrote: On Tue, 2008-03-18 at 11:37 +0100, Helmut Schaa wrote: Nevertheless I want to give you an hypothetical scenario why I would prefer it to be set in real_act_stage3_ip_config_start: As far as I can see nm_device_set_use_dhcp is defined in nm_device.h and could therefore be used from some places outside nm_device.cpp. It would be possible to call nm_device_set_use_dhcp even if the device is down. Afterwards starting a non-dhcp connection on that device would lead to the initial problem. Setting it in real_act_stage3_ip_config_start would avoid such issues. Seems kind of odd; I can't think why we'd want to set it when the device is down? The use-dhcp parameter should _only_ be set during activation, based on the 'method' parameter of the activating connection's ip4-config setting. When the activation cancels, or the device is deactivated, the use-dhcp setting should be cleared, because the next time the device activates, it might have a different connection, and therefore have a different 'method' parameter in the ip4-config setting, and therefore use autoipv4 or static addressing instead of DHCP. Well, nm_device_set_use_dhcp() shouldn't really be public, it's declaration should be in nm-device-private.h instead. The only allowed callers should be NMDevice and it's subclasses. Or maybe not even subclasses and the function should be static. Probably the last one; I can't think why the subclasses would need to touch the settings, since the decisions about IP4 config method (autoip, dhcp, manual) are all made by the NMDevice superclass in real_act_stage3_ip_config_start() and real_act_stage4_get_ip4_config(). Note that as of now, the decision to do DHCP or not is currently made in stage3, and torn down in deactivate_quickly(). So I think this issue is fixed? If nm_device_set_use_dhcp is private I have no more objections :) Helmut ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] Fix manual IP config
Am Montag, 17. März 2008 18:06:18 schrieb Tambet Ingo: On Mon, Mar 17, 2008 at 11:02 AM, Helmut Schaa [EMAIL PROTECTED] wrote: Why not call it in real_act_stage3_ip_config_start as it is done when using DHCP? That would be much clearer, no? It's much cleaner to not keep dhcp manager around when a device is deactivated. It's not only cleaner, it's a bug not to do it there. The decision is up to you :) Nevertheless I want to give you an hypothetical scenario why I would prefer it to be set in real_act_stage3_ip_config_start: As far as I can see nm_device_set_use_dhcp is defined in nm_device.h and could therefore be used from some places outside nm_device.cpp. It would be possible to call nm_device_set_use_dhcp even if the device is down. Afterwards starting a non-dhcp connection on that device would lead to the initial problem. Setting it in real_act_stage3_ip_config_start would avoid such issues. Helmut ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] Fix manual IP config
On Tue, 2008-03-18 at 11:37 +0100, Helmut Schaa wrote: Am Montag, 17. März 2008 18:06:18 schrieb Tambet Ingo: On Mon, Mar 17, 2008 at 11:02 AM, Helmut Schaa [EMAIL PROTECTED] wrote: Why not call it in real_act_stage3_ip_config_start as it is done when using DHCP? That would be much clearer, no? It's much cleaner to not keep dhcp manager around when a device is deactivated. It's not only cleaner, it's a bug not to do it there. The decision is up to you :) Nevertheless I want to give you an hypothetical scenario why I would prefer it to be set in real_act_stage3_ip_config_start: As far as I can see nm_device_set_use_dhcp is defined in nm_device.h and could therefore be used from some places outside nm_device.cpp. It would be possible to call nm_device_set_use_dhcp even if the device is down. Afterwards starting a non-dhcp connection on that device would lead to the initial problem. Setting it in real_act_stage3_ip_config_start would avoid such issues. Seems kind of odd; I can't think why we'd want to set it when the device is down? The use-dhcp parameter should _only_ be set during activation, based on the 'method' parameter of the activating connection's ip4-config setting. When the activation cancels, or the device is deactivated, the use-dhcp setting should be cleared, because the next time the device activates, it might have a different connection, and therefore have a different 'method' parameter in the ip4-config setting, and therefore use autoipv4 or static addressing instead of DHCP. Dan ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] Fix manual IP config
On Tue, Mar 18, 2008 at 12:36 PM, Dan Williams [EMAIL PROTECTED] wrote: On Tue, 2008-03-18 at 11:37 +0100, Helmut Schaa wrote: Nevertheless I want to give you an hypothetical scenario why I would prefer it to be set in real_act_stage3_ip_config_start: As far as I can see nm_device_set_use_dhcp is defined in nm_device.h and could therefore be used from some places outside nm_device.cpp. It would be possible to call nm_device_set_use_dhcp even if the device is down. Afterwards starting a non-dhcp connection on that device would lead to the initial problem. Setting it in real_act_stage3_ip_config_start would avoid such issues. Seems kind of odd; I can't think why we'd want to set it when the device is down? The use-dhcp parameter should _only_ be set during activation, based on the 'method' parameter of the activating connection's ip4-config setting. When the activation cancels, or the device is deactivated, the use-dhcp setting should be cleared, because the next time the device activates, it might have a different connection, and therefore have a different 'method' parameter in the ip4-config setting, and therefore use autoipv4 or static addressing instead of DHCP. Well, nm_device_set_use_dhcp() shouldn't really be public, it's declaration should be in nm-device-private.h instead. The only allowed callers should be NMDevice and it's subclasses. Or maybe not even subclasses and the function should be static. Tambet ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] Fix manual IP config
On Tue, 2008-03-18 at 12:46 -0600, Tambet Ingo wrote: On Tue, Mar 18, 2008 at 12:36 PM, Dan Williams [EMAIL PROTECTED] wrote: On Tue, 2008-03-18 at 11:37 +0100, Helmut Schaa wrote: Nevertheless I want to give you an hypothetical scenario why I would prefer it to be set in real_act_stage3_ip_config_start: As far as I can see nm_device_set_use_dhcp is defined in nm_device.h and could therefore be used from some places outside nm_device.cpp. It would be possible to call nm_device_set_use_dhcp even if the device is down. Afterwards starting a non-dhcp connection on that device would lead to the initial problem. Setting it in real_act_stage3_ip_config_start would avoid such issues. Seems kind of odd; I can't think why we'd want to set it when the device is down? The use-dhcp parameter should _only_ be set during activation, based on the 'method' parameter of the activating connection's ip4-config setting. When the activation cancels, or the device is deactivated, the use-dhcp setting should be cleared, because the next time the device activates, it might have a different connection, and therefore have a different 'method' parameter in the ip4-config setting, and therefore use autoipv4 or static addressing instead of DHCP. Well, nm_device_set_use_dhcp() shouldn't really be public, it's declaration should be in nm-device-private.h instead. The only allowed callers should be NMDevice and it's subclasses. Or maybe not even subclasses and the function should be static. Probably the last one; I can't think why the subclasses would need to touch the settings, since the decisions about IP4 config method (autoip, dhcp, manual) are all made by the NMDevice superclass in real_act_stage3_ip_config_start() and real_act_stage4_get_ip4_config(). Note that as of now, the decision to do DHCP or not is currently made in stage3, and torn down in deactivate_quickly(). So I think this issue is fixed? Dan ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
[PATCH] Fix manual IP config
Hi, I had some problems with manual IP configuration being ignored by NM. Attached patch for trunk fixes it for me. Regards, Helmut Index: src/nm-device.c === --- src/nm-device.c (Revision 3469) +++ src/nm-device.c (Arbeitskopie) @@ -559,6 +559,8 @@ } else ret = NM_ACT_STAGE_RETURN_FAILURE; } + else + nm_device_set_use_dhcp (self, FALSE); return ret; } ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] Fix manual IP config
2008/3/17 Helmut Schaa [EMAIL PROTECTED]: I had some problems with manual IP configuration being ignored by NM. Attached patch for trunk fixes it for me. I don't understand why or how this patch could fix the problem though. The call you added, nm_device_set_use_dhcp (self, FALSE); makes sure there's no DHCP manager associated with the device and that's already the state NMDevice has prior to activation state 3. Tambet ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] Fix manual IP config
Am Montag, 17. März 2008 17:14:34 schrieb Tambet Ingo: 2008/3/17 Helmut Schaa [EMAIL PROTECTED]: I had some problems with manual IP configuration being ignored by NM. Attached patch for trunk fixes it for me. I don't understand why or how this patch could fix the problem though. The call you added, nm_device_set_use_dhcp (self, FALSE); makes sure there's no DHCP manager associated with the device and that's already the state NMDevice has prior to activation state 3. nm_device_get_use_dhcp returns true when priv-dhcp_manager != NULL. 1. Since priv-dhcp_manager does not get initialized anywhere it may be != NULL but not set to a correct value. Therefore nm_device_get_use_dhcp will return true even when not using dhcp. Setting it explicitly with nm_device_set_use_dhcp (self, FALSE) works around this issue. 2. Assume a DHCP-connection on that device. When you switch to a manual connection priv-dhcp_manager remains set and therefor the manual IP configuration is ignored. Helmut ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] Fix manual IP config
Am Montag, 17. März 2008 17:39:12 schrieb Tambet Ingo: On Mon, Mar 17, 2008 at 10:32 AM, Helmut Schaa [EMAIL PROTECTED] wrote: 2. Assume a DHCP-connection on that device. When you switch to a manual connection priv-dhcp_manager remains set and therefor the manual IP configuration is ignored. Ah, so that's the real bug. When a device gets deactivated, it should call that function. I'll make that change today, thanks for figuring it out. Why not call it in real_act_stage3_ip_config_start as it is done when using DHCP? That would be much clearer, no? Helmut ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] Fix manual IP config
On Mon, Mar 17, 2008 at 11:02 AM, Helmut Schaa [EMAIL PROTECTED] wrote: Why not call it in real_act_stage3_ip_config_start as it is done when using DHCP? That would be much clearer, no? It's much cleaner to not keep dhcp manager around when a device is deactivated. It's not only cleaner, it's a bug not to do it there. Tambet ___ NetworkManager-list mailing list NetworkManager-list@gnome.org http://mail.gnome.org/mailman/listinfo/networkmanager-list
