Cleanup loader STEP_IP failure recovery

[James Laska]

Greetings,

While talking to Richard Hughes about a 'preupgrade' use case that requires
networking to download stage#2 during the install process, we discussed adding
boot options "ksdevice=link ip=dhcp" to get things working with no user prompts
for 95% of users.  For the other 5% that [do,can] not use DHCP, loader doesn't
currently let you go back and choose a different interface or tweak the IP
parameters once you boot with "ksdevice=____ and ip=____".  Basically, you are
presented with a dialog to [Retry] which won't work unless there is a DHCP
server responding.

To explore, and potentially correct this behavior, I tweaked net.c and loader.c
to allow you to manually walk through STEP_IFACE and STEP_IP in the event that
configuring the network has failed.  One problem was that configureTCPIP() was
using FL_IP_PARAM(flags) and FL_IPV6_PARAM(flags) to determine whether or not
to setup the network.  However, if those parameters didn't work, we never
cleared them when network activation failed.  Along with that fix, there are a
few spots where we needed to clear the values so the user could be re-prompted
for networking information.  Also, I've adjusted that to instead use
iface->ipv4Method and iface->ipv6Method.  There are a few other logMessage()
additions I found helpful when tracking the flow.

I've tested this out using a virtual guest with multiple NICs, no dhcp server
responding, different combinations of IPv6 and IPv4 networking, and networking
provided as boot args and kickstart.  The attached patch seems to have held up.
I'm sure I'm forgetting something, so I thought I'd send this out to the list
for feedback.

Thanks,
James

[PATCH] Allow loader to re-prompt for networking when network activation fails

[James Laska]

---
 loader/loader.c |   29 +++++++++++++++++++++++++++--
 loader/net.c    |   20 ++++++++++++++------
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/loader/loader.c b/loader/loader.c
index f9d5483..4675bb8 100644
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -1482,6 +1482,8 @@ static void doLoaderMain(struct loaderData_s *loaderData,
                 if (rc == LOADER_BACK || rc == LOADER_ERROR || (dir == -1 && rc == LOADER_NOOP)) {
                     step = STEP_METHOD;
                     dir = -1;
+                    /* Clear current device selection so we prompt on re-entry to STEP_IFACE */
+                    loaderData->netDev_set = 0;
                     break;
                 }

@@ -1505,7 +1507,6 @@ static void doLoaderMain(struct loaderData_s *loaderData,
                     doExit(EXIT_FAILURE);
                 }

-                logMessage(INFO, "going to do getNetConfig");

/* s390 provides all config info by way of linuxrc.s390 */
                 if (FL_HAVE_CMSCONF(flags)) {
@@ -1516,7 +1517,9 @@ static void doLoaderMain(struct loaderData_s *loaderData,
                 }

/* populate netDev based on any kickstart data */
+                logMessage(DEBUGLVL, "in doLoaderMain, calling setupIfaceStruct()");
                 setupIfaceStruct(&iface, loaderData);
+                logMessage(DEBUGLVL, "in doLoaderMain, calling readNetConfig()");
                 rc = readNetConfig(devName, &iface, loaderData->netCls, loaderData->method);

/* set the hostname if we have that */
@@ -1531,15 +1534,37 @@ static void doLoaderMain(struct loaderData_s *loaderData,
                 free(ret);
                 ret = NULL;

+                /* Go to STEP_IFACE */
                 if (rc == LOADER_BACK || (dir == -1 && rc == LOADER_NOOP)) {
                     needsNetwork = 1;
                     step = STEP_IFACE;
                     dir = -1;
+
+                   /* Clear out netDev_set so we prompt the user in
+                    * STEP_IFACE */
+                   loaderData->netDev_set = 0;
+                   /* Clear ipinfo_set so we prompt the user in when they
+                    * return to STEP_IP */
+                   loaderData->ipinfo_set = 0;
+#ifdef ENABLE_IPV6
+                   loaderData->ipv6info_set = 0;
+#endif
+                   logMessage(DEBUGLVL, "in STEP_IP, going to STEP_IFACE (LOADER_BACK)");
+
                     break;
                 }
-                /* retry */
+                /* Retry STEP_IP */
                 if (rc == LOADER_ERROR) {
                     needsNetwork = 1;
+
+                   /* Don't retry the same failed settings.  Clear ipinfo-set
+                    * so the user can enter new data */
+                   loaderData->ipinfo_set = 0;
+#ifdef ENABLE_IPV6
+                   loaderData->ipv6info_set = 0;
+#endif
+                   logMessage(DEBUGLVL, "in STEP_IP, retry (LOADER_ERROR)");
+
                     break;
                 }

diff --git a/loader/net.c b/loader/net.c
index 1f0c2a9..b0d6f46 100644
--- a/loader/net.c
+++ b/loader/net.c
@@ -513,7 +513,10 @@ int readNetConfig(char * device, iface_t * iface,
             newtWinMessage(_("Network Error"), _("Retry"),
                            _("There was an error configuring your network "
                              "interface."));
-            return LOADER_BACK;
+            /* Clear out ip selections to allow for re-entry */
+            iface->ipv4method = IPV4_UNUSED_METHOD;
+            iface->ipv6method = IPV6_UNUSED_METHOD;
+            return LOADER_ERROR;
         }

return LOADER_NOOP;
@@ -568,6 +571,7 @@ int readNetConfig(char * device, iface_t * iface,
         newtWinMessage(_("Network Error"), _("Retry"),
                        _("There was an error configuring your network "
                          "interface."));
+        /* Clear out selections to allow for re-entry */
         iface->ipv4method = IPV4_UNUSED_METHOD;
         iface->ipv6method = IPV6_UNUSED_METHOD;
         return LOADER_ERROR;
@@ -676,17 +680,19 @@ int configureTCPIP(char * device, iface_t * iface,
      *     ip=<val> noipv6
      *     ipv6=<val> noipv4
      */
-    if ((FL_IP_PARAM(flags) && FL_IPV6_PARAM(flags)) ||
-        (FL_IP_PARAM(flags) && FL_NOIPV6(flags)) ||
-        (FL_IPV6_PARAM(flags) && FL_NOIPV4(flags)) ||
-        (FL_NOIPV4(flags) && FL_NOIPV6(flags))) {
+    if ((iface->ipv4method > IPV4_UNUSED_METHOD && iface->ipv6method > IPV6_UNUSED_METHOD) || /* both */
+        (iface->ipv4method > IPV4_UNUSED_METHOD && FL_NOIPV6(flags)) || /* only ipv4 */
+        (FL_NOIPV4(flags) && iface->ipv6method > IPV6_UNUSED_METHOD) || /* only ipv6 */
+        (FL_NOIPV4(flags) && FL_NOIPV6(flags))) { /* neither ipv4 or ipv6 -- what else? */
         skipForm = 1;
         newtPopWindow();
+        logMessage(DEBUGLVL, "in configureTCPIP(), detected network boot args, skipping form");
     }
 #else
-    if (FL_IP_PARAM(flags) || FL_NOIPV4(flags)) {
+    if (iface->ipv4method > IPV4_UNUSED_METHOD || FL_NOIPV4(flags)) {
         skipForm = 1;
         newtPopWindow();
+        logMessage(DEBUGLVL, "in configureTCPIP(), detected network boot args, skipping form");
     }
 #endif

@@ -2101,7 +2107,9 @@ int kickstartNetworkUp(struct loaderData_s * loaderData, iface_t * iface) {
                            __func__, rc);
             }

+            /* Forget network device so we prompt the user */
             loaderData->netDev_set = 0;
+            /* Forget IP information so we prompt the user */
             loaderData->ipinfo_set = 0;
             free(loaderData->ipv4);
             loaderData->ipv4 = NULL;

[PATCH] Allow loader to re-prompt for networking when network activation fails

[Radek Vykydal]

Looks good to me. It is a fragile part of code, but you
gave it a lot of testing, so ack from me.

On 11/02/2010 09:42 PM, James Laska wrote:
> loader/loader.c |   29 +++++++++++++++++++++++++++--
>   loader/net.c    |   20 ++++++++++++++------
>   2 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/loader/loader.c b/loader/loader.c
> index f9d5483..4675bb8 100644
> --- a/loader/loader.c
> +++ b/loader/loader.c
> @@ -1482,6 +1482,8 @@ static void doLoaderMain(struct loaderData_s *loaderData,
>                   if (rc == LOADER_BACK || rc == LOADER_ERROR || (dir == -1&&  rc == LOADER_NOOP)) {
>                       step = STEP_METHOD;
>                       dir = -1;
>                /* Clear current device selection so we prompt on re-entry to STEP_IFACE */
>                loaderData->netDev_set = 0;
>                 break;
>             }
>
> @@ -1505,7 +1507,6 @@ static void doLoaderMain(struct loaderData_s *loaderData,
>                       doExit(EXIT_FAILURE);
>                   }
>            logMessage(INFO, "going to do getNetConfig");
>
>             /* s390 provides all config info by way of linuxrc.s390 */
>             if (FL_HAVE_CMSCONF(flags)) {
>
> @@ -1516,7 +1517,9 @@ static void doLoaderMain(struct loaderData_s *loaderData,
>                   }
>               /* populate netDev based on any kickstart data */
>            logMessage(DEBUGLVL, "in doLoaderMain, calling setupIfaceStruct()");
>             setupIfaceStruct(&iface, loaderData);
>            logMessage(DEBUGLVL, "in doLoaderMain, calling readNetConfig()");
>             rc = readNetConfig(devName,&iface, loaderData->netCls, loaderData->method);
>
>             /* set the hostname if we have that */
>
> @@ -1531,15 +1534,37 @@ static void doLoaderMain(struct loaderData_s *loaderData,
>                   free(ret);
>                   ret = NULL;
>            /* Go to STEP_IFACE */
>             if (rc == LOADER_BACK || (dir == -1&&  rc == LOADER_NOOP)) {
>                 needsNetwork = 1;
>                 step = STEP_IFACE;
>                 dir = -1;
>               /* Clear out netDev_set so we prompt the user in
>                * STEP_IFACE */
>               loaderData->netDev_set = 0;
>               /* Clear ipinfo_set so we prompt the user in when they
>                * return to STEP_IP */
>               loaderData->ipinfo_set = 0;
>
> +#ifdef ENABLE_IPV6
>               loaderData->ipv6info_set = 0;
>
> +#endif
>               logMessage(DEBUGLVL, "in STEP_IP, going to STEP_IFACE (LOADER_BACK)");
>                 break;
>             }
>            /* retry */
>            /* Retry STEP_IP */
>             if (rc == LOADER_ERROR) {
>                 needsNetwork = 1;
>               /* Don't retry the same failed settings.  Clear ipinfo-set
>                * so the user can enter new data */
>               loaderData->ipinfo_set = 0;
>
> +#ifdef ENABLE_IPV6
>               loaderData->ipv6info_set = 0;
>
> +#endif
>               logMessage(DEBUGLVL, "in STEP_IP, retry (LOADER_ERROR)");
>                 break;
>             }
>
> diff --git a/loader/net.c b/loader/net.c
> index 1f0c2a9..b0d6f46 100644
> --- a/loader/net.c
> +++ b/loader/net.c
> @@ -513,7 +513,10 @@ int readNetConfig(char * device, iface_t * iface,
>               newtWinMessage(_("Network Error"), _("Retry"),
>                              _("There was an error configuring your network "
>                                "interface."));
>        return LOADER_BACK;
>        /* Clear out ip selections to allow for re-entry */
>        iface->ipv4method = IPV4_UNUSED_METHOD;
>        iface->ipv6method = IPV6_UNUSED_METHOD;
>        return LOADER_ERROR;
>     }
>
>     return LOADER_NOOP;
>
> @@ -568,6 +571,7 @@ int readNetConfig(char * device, iface_t * iface,
>           newtWinMessage(_("Network Error"), _("Retry"),
>                          _("There was an error configuring your network "
>                            "interface."));
>    /* Clear out selections to allow for re-entry */
>     iface->ipv4method = IPV4_UNUSED_METHOD;
>     iface->ipv6method = IPV6_UNUSED_METHOD;
>     return LOADER_ERROR;
>
> @@ -676,17 +680,19 @@ int configureTCPIP(char * device, iface_t * iface,
>        *     ip=<val>  noipv6
>        *     ipv6=<val>  noipv4
>        */
> if ((FL_IP_PARAM(flags)&&  FL_IPV6_PARAM(flags)) ||
>    (FL_IP_PARAM(flags)&&  FL_NOIPV6(flags)) ||
>    (FL_IPV6_PARAM(flags)&&  FL_NOIPV4(flags)) ||
>    (FL_NOIPV4(flags)&&  FL_NOIPV6(flags))) {
> if ((iface->ipv4method>  IPV4_UNUSED_METHOD&&  iface->ipv6method>  IPV6_UNUSED_METHOD) || /* both */
>    (iface->ipv4method>  IPV4_UNUSED_METHOD&&  FL_NOIPV6(flags)) || /* only ipv4 */
>    (FL_NOIPV4(flags)&&  iface->ipv6method>  IPV6_UNUSED_METHOD) || /* only ipv6 */
>    (FL_NOIPV4(flags)&&  FL_NOIPV6(flags))) { /* neither ipv4 or ipv6 -- what else? */
>     skipForm = 1;
>     newtPopWindow();
>    logMessage(DEBUGLVL, "in configureTCPIP(), detected network boot args, skipping form");
> }
> #else
> if (FL_IP_PARAM(flags) || FL_NOIPV4(flags)) {
> if (iface->ipv4method>  IPV4_UNUSED_METHOD || FL_NOIPV4(flags)) {
>      skipForm = 1;
>      newtPopWindow();
>    logMessage(DEBUGLVL, "in configureTCPIP(), detected network boot args, skipping form");
> }
> #endif
>
> @@ -2101,7 +2107,9 @@ int kickstartNetworkUp(struct loaderData_s * loaderData, iface_t * iface) {
>                              __func__, rc);
>               }
>        /* Forget network device so we prompt the user */
>         loaderData->netDev_set = 0;
>        /* Forget IP information so we prompt the user */
>         loaderData->ipinfo_set = 0;
>         free(loaderData->ipv4);
>         loaderData->ipv4 = NULL;

[PATCH] Allow loader to re-prompt for networking when network activation fails

[Chris Lumens]

> @@ -676,17 +680,19 @@ int configureTCPIP(char * device, iface_t * iface,
>       *     ip=<val> noipv6
>       *     ipv6=<val> noipv4
>       */
> if ((FL_IP_PARAM(flags) && FL_IPV6_PARAM(flags)) ||
>    (FL_IP_PARAM(flags) && FL_NOIPV6(flags)) ||
>    (FL_IPV6_PARAM(flags) && FL_NOIPV4(flags)) ||
>    (FL_NOIPV4(flags) && FL_NOIPV6(flags))) {
> if ((iface->ipv4method > IPV4_UNUSED_METHOD && iface->ipv6method > IPV6_UNUSED_METHOD) || /* both */
>    (iface->ipv4method > IPV4_UNUSED_METHOD && FL_NOIPV6(flags)) || /* only ipv4 */
>    (FL_NOIPV4(flags) && iface->ipv6method > IPV6_UNUSED_METHOD) || /* only ipv6 */
>    (FL_NOIPV4(flags) && FL_NOIPV6(flags))) { /* neither ipv4 or ipv6 -- what else? */
>    skipForm = 1;
>    newtPopWindow();
>    logMessage(DEBUGLVL, "in configureTCPIP(), detected network boot args, skipping form");
> }

Something's got to be done about this conditional.  It well past the
point of being unwieldy.

- Chris

[Radek Vykydal]

On 11/02/2010 09:42 PM, James Laska wrote:
> Greetings,
>
> While talking to Richard Hughes about a 'preupgrade' use case that requires
> networking to download stage#2 during the install process, we discussed adding
> boot options "ksdevice=link ip=dhcp" to get things working with no user prompts
> for 95% of users.  For the other 5% that [do,can] not use DHCP, loader doesn't
> currently let you go back and choose a different interface or tweak the IP
> parameters once you boot with "ksdevice=____ and ip=____".  Basically, you are
> presented with a dialog to [Retry] which won't work unless there is a DHCP
> server responding.

James, thanks for the patch, I have a Fedora bug for what it does
(https://bugzilla.redhat.com/show_bug.cgi?id=504983),
but doesn't the use case disappear with advent of Giant initrd?

Anyway it seems still relevant for stage 1 network configuration
in rhel 6, though I have no such a bug filed for rhel 6 yet.

I'll try to look at it.

Radek

[James Laska]

On Wed, 2010-11-03 at 13:44 +0100, Radek Vykydal wrote:
> On 11/02/2010 09:42 PM, James Laska wrote:
> > Greetings,
> >
> > While talking to Richard Hughes about a 'preupgrade' use case that requires
> > networking to download stage#2 during the install process, we discussed adding
> > boot options "ksdevice=link ip=dhcp" to get things working with no user prompts
> > for 95% of users.  For the other 5% that [do,can] not use DHCP, loader doesn't
> > currently let you go back and choose a different interface or tweak the IP
> > parameters once you boot with "ksdevice=____ and ip=____".  Basically, you are
> > presented with a dialog to [Retry] which won't work unless there is a DHCP
> > server responding.
>
> James, thanks for the patch, I have a Fedora bug for what it does
> (https://bugzilla.redhat.com/show_bug.cgi?id=504983),
> but doesn't the use case disappear with advent of Giant initrd?

Yes, I'm glad you asked.  That particular use case will go away with
giant initrd.  Unfortunately for preupgrade, it will might be replaced
by a harder to solve problem (see bug#646843) when 500M /boot doesn't
have enough space.
> Anyway it seems still relevant for stage 1 network configuration
> in rhel 6, though I have no such a bug filed for rhel 6 yet.

Thanks.  I think this behavior has been there for some time.  I've
typically just rebooted and adjusted my arguments, or updated the ks.cfg
to use the correct NIC.  But I think with the patch, it will avoid
having to reboot and allow the user to select a different device or IP
settings.

Thanks,
James

[Radek Vykydal]

On 11/03/2010 01:44 PM, Radek Vykydal wrote:
> On 11/02/2010 09:42 PM, James Laska wrote:
> > Greetings,
> >
> > While talking to Richard Hughes about a 'preupgrade' use case that 
> > requires
> > networking to download stage#2 during the install process, we 
> > discussed adding
> > boot options "ksdevice=link ip=dhcp" to get things working with no 
> > user prompts
> > for 95% of users.  For the other 5% that [do,can] not use DHCP, 
> > loader doesn't
> > currently let you go back and choose a different interface or tweak 
> > the IP
> > parameters once you boot with "ksdevice=____ and ip=____".  
> > Basically, you are
> > presented with a dialog to [Retry] which won't work unless there is a 
> > DHCP
> > server responding.
>
> James, thanks for the patch, I have a Fedora bug for what it does
> (https://bugzilla.redhat.com/show_bug.cgi?id=504983)

Well, I was too fast, the bug is concerning something different.

Radek