[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [master] Improve reboot modes in init.c and shutdown.c.



Hi Steffen,

On 01/29/2010 08:22 PM, Steffen Maier wrote:
@@ -438,8 +438,8 @@ int main(int argc, char **argv) {
      pid_t installpid, childpid;
      int waitStatus;
      int fd = -1;
-    int doReboot = 0;
      int doShutdown =0;
+    int shutdown_method = HALT;

reboot_action shutdown_method = HALT;
To make it virtually more type safe and more readable?

Aboslutely.

So if anaconda or loader crash, also s390 will run into the SIGINT
thingy again?

Because s390 is special, here's what I think will happen: if anaconda crashes, linuxrc.s390 receives SIGUSR1, that calls shutdown and that just halts. If loader crashes, linuxrc.s390 simply does HALT.

I think this is safe and okay for the developer, since (I guess) you can always scroll the console up and see the errors.

          printf("\n");
      } else {
-        doReboot = 1;
+        shutdown_method = REBOOT;
      }

      expected_exit = 1;

Do you still need this with the fixes in here?

I don't, away it goes.

I suppose there is at least one more call of shutDown that should use
shutdown_method instead of a hardcoded reboot_action:

/* reboot handler */
static void sigintHandler(int signum) {
     termReset();
     shutDown(getKillPolicy(), REBOOT);
}

REBOOT =>  shutdown_method
Which requires shutdown_method to be static and global to the compile
unit init.c. Otherwise, I fear, pressing CAD will unconditionally REBOOT
even if shutdown_method was set to VERBOSE_REBOOT above.

No, like you said in the other mail, we want an unconditional reboot on CAD.

+	HALT,
+        /* gives user a chance to read the trace before scrolling the text out
+           with disk unmounting and termination info */
+        VERBOSE_REBOOT

Maybe renaming to DELAYED_REBOOT would be more accurate, since it does
not cause shutdown to output more messages than with a REBOOT, it just
delays the actual reboot until sigint or ctrl-alt-del.

I agree, renamed.

+    case HALT:
+        printf("halting system\n");

I'm undecided, if a sleep(2) for consistency with the other cases is a
good or bad idea.

I didn't put the sleep in because after the system goes to halt the message stays on the screen and user can read it (along with 'system halted.' from the kernel). So no need to wait here I think. Let's wait until someone seriously complains.


+        reboot(RB_HALT_SYSTEM);
+        break;
+    default:
+        break;
+    }
  }

-void shutDown(int doKill, reboot_action rebootAction) {
-	if (doKill) {
-		performUnmounts();
-		performTerminations();
-	}
-
-	if ((rebootAction == POWEROFF || rebootAction == REBOOT)&&  doKill) {
-		performReboot(rebootAction);
-	}
+static void performVerboseReboot()
+{
+    printf("the system will be rebooted when you press Ctrl-Alt-Delete.\n");

It should also reboot, when the user presses CTRL+C or sends a SIGINT
(which ctrl-alt-del actually triggers, if I understood the code
correctly). Maybe we should also mention the CTRL+C method, which is
theoretically also possible on s390 as opposed to the CAD key combo.

You are right. I will try this and modify the prompt accordingly.


+    while (1) {
+        sleep(1);
+    }
+}

-	printf("you may safely reboot your system\n");
-	exit(0);
-	return;
+void shutDown(int doKill, reboot_action rebootAction)
+{
+    if (rebootAction == VERBOSE_REBOOT) {
+        performVerboseReboot();

Does the SIGINT/CAD to reboot still work for non-s390 now that the
rebootHandler is gone from shutdown.c?

Yes.



The other sigint handler in init.c only triggers this code path in
shutdown, but then shutdown seems to be trapped in
performVerboseReboot's endless loop.

I can see in your other post that this actually is not an issue, correct me if I am misunderstanding please.


+    } else if (doKill) {
+        performUnmounts();
+        performTerminations();
+        performReboot(rebootAction);
+    }
+    printf("you may safely reboot your system\n");
+    exit(0);
+    return;
  }

  #ifdef AS_SHUTDOWN

All of this should also go into rhel6-branch because of bug 533198.
Master and rhel6-branch would additionally need the following:
+ a new option -H for the main() of AS_SHUTDOWN to HALT
+ changing linuxrc.s390 to exec "/sbin/shutdown -H" in doshutdown()

I will send the final patch to the list again when I have the final version and ask David Cantrell to review for master and rhel6.

Thank you!

Ales


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]