Review Changes #4

canonical · Jul 6, 2023 · 0703279 · 0703279
1 parent 88ae852
commit 0703279
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 87 deletions.
diff --git a/internals/daemon/daemon.go b/internals/daemon/daemon.go
@@ -28,9 +28,9 @@ import (
 	"runtime"
 	"strings"
 	"sync"
+	"syscall"
 	"time"
 
-	"golang.org/x/sys/unix"
 	"gopkg.in/tomb.v2"
 
 	"github.com/gorilla/mux"
@@ -704,7 +704,7 @@ func (d *Daemon) doReboot(sigCh chan<- os.Signal, waitTimeout time.Duration) err
 		return err
 	}
 	// wait for reboot to happen
-	logger.Noticef("Waiting for system reboot")
+	logger.Noticef("Waiting for system reboot...")
 	if sigCh != nil {
 		signal.Stop(sigCh)
 		if len(sigCh) > 0 {
@@ -741,45 +741,27 @@ func commandReboot(rebootDelay time.Duration) error {
 }
 
 var (
-	capGetSyscall = unix.Capget
-	syncSyscall   = unix.Sync
-	rebootSyscall = unix.Reboot
+	syncSyscall   = syscall.Sync
+	rebootSyscall = syscall.Reboot
 )
 
 // syscallReboot performs a delayed async reboot using direct Linux
 // kernel syscalls.
 //
 // Note: Reboot message not currently supported.
 func syscallReboot(rebootDelay time.Duration) error {
-	var caps unix.CapUserData
-	// We deliberately use v1 caps here due to:
-	// https://github.com/golang/go/issues/44312
-	hdr := unix.CapUserHeader{Version: unix.LINUX_CAPABILITY_VERSION_1}
-	err := capGetSyscall(&hdr, &caps)
-	if err != nil {
-		return err
-	}
-	if (int32(caps.Effective) & (1 << unix.CAP_SYS_BOOT)) == 0 {
-		return fmt.Errorf("no capability to reboot")
-	}
-
 	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() {
-		// As per the requirements of the reboot syscall, we have to
-		// first call sync.
+		// As per the requirements of the reboot syscall, we
+		// have to first call sync.
 		syncSyscall()
-		// This syscall can fail (EINVAL/EPERM) if invalid arguments are
-		// supplied or CAP_SYS_BOOT capability is missing. We cover the
-		// latter case in a separate capability check, so this will not
-		// happen here, but let's panic if we see something to make sure
-		// we catch anything unexpected.
-		err := rebootSyscall(unix.LINUX_REBOOT_CMD_RESTART)
+		err := rebootSyscall(syscall.LINUX_REBOOT_CMD_RESTART)
 		if err != nil {
-			panic("internal error: reboot syscall failed")
+			logger.Noticef("reboot syscall failed : %v", err)
 		}
 	})
 	return nil
@@ -881,9 +863,9 @@ func getListener(socketPath string, listenerMap map[string]net.Listener) (net.Li
 	}
 
 	runtime.LockOSThread()
-	oldmask := unix.Umask(0111)
+	oldmask := syscall.Umask(0111)
 	listener, err := net.ListenUnix("unix", address)
-	unix.Umask(oldmask)
+	syscall.Umask(oldmask)
 	runtime.UnlockOSThread()
 	if err != nil {
 		return nil, err

diff --git a/internals/daemon/daemon_test.go b/internals/daemon/daemon_test.go
@@ -29,14 +29,13 @@ import (
 	"testing"
 	"time"
 
-	"golang.org/x/sys/unix"
 	"github.com/gorilla/mux"
-
 	"gopkg.in/check.v1"
 
 	// XXX Delete import above and make this file like the other ones.
 	. "gopkg.in/check.v1"
 
+	"github.com/canonical/pebble/internals/logger"
 	"github.com/canonical/pebble/internals/osutil"
 	"github.com/canonical/pebble/internals/overlord/patch"
 	"github.com/canonical/pebble/internals/overlord/restart"
@@ -1166,26 +1165,11 @@ func mockRebootSyscall(f func(cmd int) error) (restore func()) {
 	}
 }
 
-func mockCapGetSyscall(f func(hdr *unix.CapUserHeader, data *unix.CapUserData) error) (restore func()) {
-	old := capGetSyscall
-	capGetSyscall = f
-	return func() {
-		capGetSyscall = old
-	}
-}
-
-
 func (s *daemonSuite) TestSyscallPosRebootDelay(c *C) {
 	wait := make(chan int)
-	defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error {
-		if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 {
-			data.Effective = 0xFFFFFFFF
-		}
-		return nil
-	})()
 	defer mockSyncSyscall(func() {})()
 	defer mockRebootSyscall(func(cmd int) error {
-		if cmd == unix.LINUX_REBOOT_CMD_RESTART {
+		if cmd == syscall.LINUX_REBOOT_CMD_RESTART {
 			wait <- 1
 		}
 		return nil
@@ -1206,15 +1190,9 @@ func (s *daemonSuite) TestSyscallPosRebootDelay(c *C) {
 
 func (s *daemonSuite) TestSyscallNegRebootDelay(c *C) {
 	wait := make(chan int)
-	defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error {
-		if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 {
-			data.Effective = 0xFFFFFFFF
-		}
-		return nil
-	})()
 	defer mockSyncSyscall(func() {})()
 	defer mockRebootSyscall(func(cmd int) error {
-		if cmd == unix.LINUX_REBOOT_CMD_RESTART {
+		if cmd == syscall.LINUX_REBOOT_CMD_RESTART {
 			wait <- 1
 		}
 		return nil
@@ -1238,15 +1216,9 @@ func (s *daemonSuite) TestSyscallNegRebootDelay(c *C) {
 
 func (s *daemonSuite) TestSetSyscall(c *C) {
 	wait := make(chan int)
-	defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error {
-		if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 {
-			data.Effective = 0xFFFFFFFF
-		}
-		return nil
-	})()
 	defer mockSyncSyscall(func() {})()
 	defer mockRebootSyscall(func(cmd int) error {
-		if cmd == unix.LINUX_REBOOT_CMD_RESTART {
+		if cmd == syscall.LINUX_REBOOT_CMD_RESTART {
 			wait <- 1
 		}
 		return nil
@@ -1270,44 +1242,45 @@ func (s *daemonSuite) TestSetSyscall(c *C) {
 	}
 }
 
-func (s *daemonSuite) TestCapSysBootFail(c *C) {
-	defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error {
-		if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 {
-			data.Effective = ^(uint32(1 << unix.CAP_SYS_BOOT))
-		}
-		return fmt.Errorf("get cap syscall failed")
-	})()
-	defer mockSyncSyscall(func() {})()
-	defer mockRebootSyscall(func(cmd int) error { return nil })()
-
-	// We know the default is commandReboot otherwise the unit tests
-	// above will fail. We need to check the switch works.
-	SetSyscallReboot()
-	defer func() {
-		rebootHandler = commandReboot
-	}()
+type fakeLogger struct {
+	s string
+	c chan int
+}
 
-	err := rebootHandler(0)
-	c.Assert(err, ErrorMatches, "get cap syscall failed")
+func (f *fakeLogger) Notice(msg string) {
+	f.s = msg
+	f.c <- 1
 }
 
-func (s *daemonSuite) TestCapSysBootNotAvail(c *C) {
-	defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error {
-		if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 {
-			data.Effective = ^(uint32(1 << unix.CAP_SYS_BOOT))
-		}
-		return nil
-	})()
+func (f *fakeLogger) Debug(msg string) {}
+
+func (s *daemonSuite) TestSyscallRebootError(c *C) {
 	defer mockSyncSyscall(func() {})()
-	defer mockRebootSyscall(func(cmd int) error { return nil })()
+	defer mockRebootSyscall(func(cmd int) error {
+		return fmt.Errorf("-EPERM")
+	})()
 
 	// We know the default is commandReboot otherwise the unit tests
 	// above will fail. We need to check the switch works.
 	SetSyscallReboot()
 	defer func() {
 		rebootHandler = commandReboot
 	}()
+	complete := make(chan int)
+	l := fakeLogger{c: complete}
+	old := logger.SetLogger(&l)
+	defer func() {
+		logger.SetLogger(old)
+	}()
 
 	err := rebootHandler(0)
-	c.Assert(err, ErrorMatches, "no reboot cap")
+	c.Assert(err, IsNil)
+	// This would block forever if the switch did not work.
+	timeout := time.Second * 10
+	select {
+	case <-complete:
+	case <-time.After(timeout): // exit test if we fail and get stuck
+		c.Fail()
+	}
+	c.Assert(l.s, Matches, "*-EPERM")
 }