Review Changes #7

internals/daemon/daemon.go

@@ -712,16 +712,30 @@ func (d *Daemon) doReboot(sigCh chan<- os.Signal, waitTimeout time.Duration) err
 
 const rebootMsg = "reboot scheduled to update the system"
 
-var rebootHandler = commandReboot
+var rebootHandler = systemdModeReboot
 
-// SetSyscallReboot replaces the default command-based reboot
-// with a direct Linux kernel syscall based implementation.
-func SetSyscallReboot() {
-	rebootHandler = syscallReboot
+type RebootMode int
+
+const (
+	SystemdMode RebootMode = iota + 1
+	SyscallMode
+)
+
+// SetRebootMode can set how the system should issue a reboot.
+// The default reboot handler mode is SystemdMode.
+func SetRebootMode(mode RebootMode) {
+	switch mode {
+	case SystemdMode:
+		rebootHandler = systemdModeReboot
+	case SyscallMode:
+		rebootHandler = syscallModeReboot
+	default:
+		panic(fmt.Sprintf("unsupported reboot mode %v", mode))
+	}
 }
 
-// commandReboot assumes a userspace shutdown command exists.
-func commandReboot(rebootDelay time.Duration) error {
+// systemdModeReboot assumes a userspace shutdown command exists.
+func systemdModeReboot(rebootDelay time.Duration) error {
 	if rebootDelay < 0 {
 		rebootDelay = 0
 	}
@@ -734,29 +748,35 @@ func commandReboot(rebootDelay time.Duration) error {
 }
 
 var (
-	syncSyscall   = syscall.Sync
-	rebootSyscall = syscall.Reboot
+	syscallSync   = syscall.Sync
+	syscallReboot = syscall.Reboot
 )
 
-// syscallReboot performs a delayed async reboot using direct Linux
-// kernel syscalls.
+// syscallModeReboot performs a non-blocking delayed reboot using direct Linux
+// kernel syscalls. If the delay is negative or zero, the reboot is issued
+// immediately.
 //
 // Note: Reboot message not currently supported.
-func syscallReboot(rebootDelay time.Duration) error {
-	if rebootDelay < 0 {
-		rebootDelay = 0
-	}
-	// This has to be non-blocking, and scheduled for a future
-	// point in time to mimic shutdown.
-	time.AfterFunc(rebootDelay, func() {
+func syscallModeReboot(rebootDelay time.Duration) error {
+	safeReboot := func() {
 		// As per the requirements of the reboot syscall, we
 		// have to first call sync.
-		syncSyscall()
-		err := rebootSyscall(syscall.LINUX_REBOOT_CMD_RESTART)
+		syscallSync()
+		err := syscallReboot(syscall.LINUX_REBOOT_CMD_RESTART)
 		if err != nil {
 			logger.Noticef("Failed on reboot syscall: %v", err)
 		}
-	})
+	}
+
+	if rebootDelay <= 0 {
+		// Synchronous reboot right now.
+		safeReboot()
+	} else {
+		// Asynchronous non-blocking reboot scheduled
+		time.AfterFunc(rebootDelay, func() {
+			safeReboot()
+		})
+	}
 	return nil
 }
 

internals/daemon/daemon_test.go

@@ -694,7 +694,7 @@ func (s *daemonSuite) TestRestartSystemWiring(c *C) {
 	oldRebootNoticeWait := rebootNoticeWait
 	oldRebootWaitTimeout := rebootWaitTimeout
 	defer func() {
-		rebootHandler = commandReboot
+		rebootHandler = systemdModeReboot
 		rebootNoticeWait = oldRebootNoticeWait
 		rebootWaitTimeout = oldRebootWaitTimeout
 	}()
@@ -1182,34 +1182,18 @@ type rebootSuite struct{}
 
 var _ = Suite(&rebootSuite{})
 
-func mockSyncSyscall(f func()) (restore func()) {
-	old := syncSyscall
-	syncSyscall = f
-	return func() {
-		syncSyscall = old
-	}
-}
-
-func mockRebootSyscall(f func(cmd int) error) (restore func()) {
-	old := rebootSyscall
-	rebootSyscall = f
-	return func() {
-		rebootSyscall = old
-	}
-}
-
 func (s *rebootSuite) TestSyscallPosRebootDelay(c *C) {
 	wait := make(chan int)
-	defer mockSyncSyscall(func() {})()
-	defer mockRebootSyscall(func(cmd int) error {
+	defer FakeSyscallSync(func() {})()
+	defer FakeSyscallReboot(func(cmd int) error {
 		if cmd == syscall.LINUX_REBOOT_CMD_RESTART {
 			wait <- 1
 		}
 		return nil
 	})()
 
 	period := 25 * time.Millisecond
-	syscallReboot(period)
+	syscallModeReboot(period)
 	start := time.Now()
 	select {
 	case <-wait:
@@ -1222,8 +1206,8 @@ func (s *rebootSuite) TestSyscallPosRebootDelay(c *C) {
 
 func (s *rebootSuite) TestSyscallNegRebootDelay(c *C) {
 	wait := make(chan int)
-	defer mockSyncSyscall(func() {})()
-	defer mockRebootSyscall(func(cmd int) error {
+	defer FakeSyscallSync(func() {})()
+	defer FakeSyscallReboot(func(cmd int) error {
 		if cmd == syscall.LINUX_REBOOT_CMD_RESTART {
 			wait <- 1
 		}
@@ -1235,7 +1219,10 @@ func (s *rebootSuite) TestSyscallNegRebootDelay(c *C) {
 	// effectively a race, but given the huge timeout, it is not going
 	// to be a problem (c).
 	period := 10 * time.Second
-	syscallReboot(-period)
+	go func() {
+		// We need a different thread for the unbuffered wait.
+		syscallModeReboot(-period)
+	}()
 	start := time.Now()
 	select {
 	case <-wait:
@@ -1248,28 +1235,30 @@ func (s *rebootSuite) TestSyscallNegRebootDelay(c *C) {
 
 func (s *rebootSuite) TestSetSyscall(c *C) {
 	wait := make(chan int)
-	defer mockSyncSyscall(func() {})()
-	defer mockRebootSyscall(func(cmd int) error {
+	defer FakeSyscallSync(func() {})()
+	defer FakeSyscallReboot(func(cmd int) error {
 		if cmd == syscall.LINUX_REBOOT_CMD_RESTART {
 			wait <- 1
 		}
 		return nil
 	})()
 
-	// We know the default is commandReboot otherwise the unit tests
+	// We know the default is systemdReboot otherwise the unit tests
 	// above will fail. We need to check the switch works.
-	SetSyscallReboot()
-	defer func() {
-		rebootHandler = commandReboot
-	}()
+	SetRebootMode(SyscallMode)
+	defer SetRebootMode(SystemdMode)
 
-	err := rebootHandler(0)
-	c.Assert(err, IsNil)
+	var err error
+	go func() {
+		// We need a different thread for the unbuffered wait.
+		err = rebootHandler(0)
+	}()
 	select {
 	case <-wait:
 	case <-time.After(10 * time.Second):
 		c.Fatal("syscall did not take place and we timed out")
 	}
+	c.Assert(err, IsNil)
 }
 
 type fakeLogger struct {
@@ -1285,28 +1274,31 @@ func (f *fakeLogger) Notice(msg string) {
 func (f *fakeLogger) Debug(msg string) {}
 
 func (s *rebootSuite) TestSyscallRebootError(c *C) {
-	defer mockSyncSyscall(func() {})()
-	defer mockRebootSyscall(func(cmd int) error {
+	defer FakeSyscallSync(func() {})()
+	defer FakeSyscallReboot(func(cmd int) error {
 		return fmt.Errorf("-EPERM")
 	})()
 
-	// We know the default is commandReboot otherwise the unit tests
+	// We know the default is systemdReboot otherwise the unit tests
 	// above will fail. We need to check the switch works.
-	SetSyscallReboot()
-	defer func() {
-		rebootHandler = commandReboot
-	}()
+	SetRebootMode(SyscallMode)
+	defer SetRebootMode(SystemdMode)
+
 	complete := make(chan int)
 	l := fakeLogger{noticeCh: complete}
 	old := logger.SetLogger(&l)
 	defer logger.SetLogger(old)
 
-	err := rebootHandler(0)
-	c.Assert(err, IsNil)
+	var err error
+	go func() {
+		// We need a different thread for the unbuffered wait.
+		err = rebootHandler(0)
+	}()
 	select {
 	case <-complete:
 	case <-time.After(10 * time.Second):
 		c.Fatal("syscall did not take place and we timed out")
 	}
+	c.Assert(err, IsNil)
 	c.Assert(l.msg, Matches, "*-EPERM")
 }

internals/daemon/export_test.go

@@ -46,3 +46,19 @@ func FakeGetChecks(f func(o *overlord.Overlord) ([]*checkstate.CheckInfo, error)
 		getChecks = old
 	}
 }
+
+func FakeSyscallSync(f func()) (restore func()) {
+	old := syscallSync
+	syscallSync = f
+	return func() {
+		syscallSync = old
+	}
+}
+
+func FakeSyscallReboot(f func(cmd int) error) (restore func()) {
+	old := syscallReboot
+	syscallReboot = f
+	return func() {
+		syscallReboot = old
+	}
+}