Re: [patch] NM does not completely handle default route

[Gene Czarcinski <gene czarc net>]

On Saturday 10 October 2009 17:56:36 Gene Czarcinski wrote:
> On Saturday 10 October 2009 17:32:42 Gene Czarcinski wrote:
> > OK, this is a followup to a lot of my previous email about NM handling
> > the default route when a system has multiple NICs.
> >
> > I have closed out my BZ report for Fedora 11 as WONTFIX --
> > https://bugzilla.redhat.com/show_bug.cgi?id=523875 -- and have opened a
> > new report against rawhide --
> >  https://bugzilla.redhat.com/show_bug.cgi?id=528281
> >
> > I do not expect that this problem will be fixed in time for F12 GA but
> > hope that it can be fixed post GA.
> >
> > I have been poking around the NM source code mainly with grep and gedit. 
> > I believe it is possible/practical to fix things with any big re-write
> > (which I believe is neither practical or desirable). [sure is a lot of
> > code]  I mainly looked at how GATEWAYDEV=, GATEWAY=, and ONBOOT= (and
> > their ~/.gconf/system/networking/ counterparts) where handled.
> >
> > Some characteristics/constraints --
> >
> > 1.  Having a system with one or more NICs and no default route is a valid
> > configuration and should (must?) be supported by NM.
> >
> > 2. Regardless of having a default route or not, some connections should
> >  never be the default route (the intent, I believe, of the current
> >  implementation).
> >
> > 3.  Having GATEWAYDEV=xxx in /etc/sysconfig/network will cause all NICs
> >  other than xxx to be marked as "never-default".  This is and should
> >  continue to be supported.
> >
> > 4. Only if a NIC is marked as NOT for all users can I mark it for
> >  "connection only" (never-default).  This needs to be fixed.  I should be
> >  able to mark a NIC which is available to all users (an ifcfg-xxx system
> >  configuration in /etc/sysconfig/network-scripts/) as a "connection only"
> >  (never-default) NIC.
> >
> > 5.  I should be able to mark a NIC as the default route device.  I think
> >  this is needed for completeness but am not sure it is really required.
> >
> > 6.  If two or more NICs with static IPs are configured with different
> >  default route, I do not care ... this is a mis-configuration.
> >
> > 7.  The problem is not really with NICs that have static IPs but with
> > those that use dhcp where each dhcp server supplies a default route.
> 
> Oops ... I sent this partial message instead of saving the draft.  To
>  continue ...
> 
> 8. system-config-network currently has a problem with GATEWAYDEV=xxx being
>  in the /etc/sysconfig/network file and will delete it if you use s-c-n to
>  change the NIC system configuration files.  Any solution needs to avoid
>  this problem. ---------------------------------------------
> What I propose is also something Dan mentioned ... add a new
>  option/parameter to the ifcfg-xxx file.  I tried adding "FOOBARBS=yes" to
>  the file and this is left alone by system-config-network and ignored by
>  NM.
> 
> So, I propose adding a new option/parameter such as "DEFAULTGW=yes|no" to
>  the system configuration file.
> 
> Upon loading the file (in ifcfg-rh/reader.c), if DEFAULTGW=no is specified,
>  then mark that connection as "never-default".
> 
> When saving the file (applying updates), if the connection is
>  "never-default", then set DEFAULTGW=no.
> 
> A small problem occurs when I am changing a connection definition for
>  "never- default" to allowing it for default.  For now, I propose that a
>  connection which is NOT "never-default" (that is, never-default is false
>  in
> ~/.gconf/...), then we should set DEFAULTGW=? for now.
> 
> As an initial attempt at a fix, I think this may be "good enough".  Yes,
>  having DEFAULTGW=yes have some meaning may be useful but ... ???.  As
>  described, I think this will only involve ifcfg-rh/reader.c and
>  ifcfg-rh/writer.c.
> 
> The patch to implement this should be fairly simple and I am going to give
>  it a try.

Attached is a proposed patch to "correct" the default route with system config 
(ifcfg-xxx) files.

I decided to follow the general approach used in the existing code which 
creates the "never-default" attribute for a NIC/connection.

The only changes are to ifcfg-rh/reader.c and ifcfg-rh/writer.c.  This change 
creates a new option/parameter for the ifcfg-xxx file in /etc/sysconfig/network-
scripts/:  NM_NEVER_DEFAULT=xxx

On input (reader.c), if NM_NEVER_DEFAULT=yes in the ifcfg-xxx file or 
GATWAYDEV=xxx in /etc/sysconfig/network indicate that this device should never 
have the default route then never-default is set to true, otherwise it is 
false.

On output: 

If never-default is true, then NM_NEVER_DEFAULT=yes is added/changed in the 
ifcfg-xxx file. 

If never-default is false and NM_NEVER_DEFAULT= is in the ifcfg-xxx file, it is 
changed to NM_NEVER_DEFAULT=no.

If never-default is false and NM_NEVER_DEFAULT= does not exist in the ifcfg-
xxx file, nothing is changed.

The GATEWAYDEV= code in reader.c remains.  However, consideration should be 
given to deleting the code since 1) it is poorly documented and 2) it is 
unnecessary with this patch.

The addition is NM_NEVER_DEFAULT= is compatible with system-config-network 
whereas GATEWAYDEV= is not compatible.

Gene

diff -uNr NetworkManager-0.7.996.old/system-settings/plugins/ifcfg-rh/reader.c NetworkManager-0.7.996/system-settings/plugins/ifcfg-rh/reader.c
--- NetworkManager-0.7.996.old/system-settings/plugins/ifcfg-rh/reader.c	2009-09-29 02:11:14.000000000 -0400
+++ NetworkManager-0.7.996/system-settings/plugins/ifcfg-rh/reader.c	2009-10-11 14:12:53.822486038 -0400
@@ -623,6 +623,11 @@
 		return NULL;
 	}
 
+    /* first, check if NM_NEVER_DEFAULT= is set for this device */
+    /* only set if it exists and ==yes */
+	never_default = svTrueValue (ifcfg, "NM_NEVER_DEFAULT", FALSE);
+
+    /* then check if GATEWAYDEV= */
 	network_ifcfg = svNewFile (network_file);
 	if (network_ifcfg) {
 		char *gatewaydev;
diff -uNr NetworkManager-0.7.996.old/system-settings/plugins/ifcfg-rh/writer.c NetworkManager-0.7.996/system-settings/plugins/ifcfg-rh/writer.c
--- NetworkManager-0.7.996.old/system-settings/plugins/ifcfg-rh/writer.c	2009-09-29 02:11:14.000000000 -0400
+++ NetworkManager-0.7.996/system-settings/plugins/ifcfg-rh/writer.c	2009-10-11 14:16:02.356811932 -0400
@@ -949,6 +949,20 @@
 	} else
 		svSetValue (ifcfg, "DOMAIN", NULL, FALSE);
 
+    /* handle NM_NEVER_DEFAULT */
+    /* this probably could be simpler but svSetVale() is confusing */
+    /* if never_default is true, set NM_NEVER_DEFAULT=yes */
+    /* if never_default is false, only set "no" if NM_NEVER_DEFAULT already exists */
+    if (nm_setting_ip4_config_get_never_default(s_ip4))
+        svSetValue (ifcfg, "NM_NEVER_DEFAULT", "yes", FALSE);
+    else {
+        tmp = svGetValue (ifcfg, "NM_NEVER_DEFAULT", FALSE);
+        if (tmp) {
+           svSetValue (ifcfg, "NM_NEVER_DEFAULT", "no", FALSE);
+        }
+        g_free (tmp);
+    }
+
 	svSetValue (ifcfg, "PEERDNS", NULL, FALSE);
 	svSetValue (ifcfg, "PEERROUTES", NULL, FALSE);
 	svSetValue (ifcfg, "DHCP_HOSTNAME", NULL, FALSE);