many: cleanup, revert the -1 support in services list for client

client/clientutil/session.go

@@ -0,0 +1,51 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2024 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package clientutil
+
+import (
+	"path/filepath"
+	"strconv"
+
+	"github.com/snapcore/snapd/dirs"
+)
+
+// AvailableUserSessions returns a list of available user-session targets for
+// snapd, by probing the available snapd-session-agent sockets in the
+// XDG runtime directory.
+func AvailableUserSessions() ([]int, error) {
+	sockets, err := filepath.Glob(filepath.Join(dirs.XdgRuntimeDirGlob, "snapd-session-agent.socket"))
+	if err != nil {
+		return nil, err
+	}
+
+	var uids []int
+	for _, sock := range sockets {
+		uidStr := filepath.Base(filepath.Dir(sock))
+		uid, err := strconv.Atoi(uidStr)
+		if err != nil {
+			// Ignore directories that do not
+			// appear to be valid XDG runtime dirs
+			// (i.e. /run/user/NNNN).
+			continue
+		}
+		uids = append(uids, uid)
+	}
+	return uids, nil
+}

client/clientutil/session_test.go

@@ -0,0 +1,76 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2024 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package clientutil_test
+
+import (
+	"os"
+	"path"
+	"sort"
+
+	"github.com/snapcore/snapd/client/clientutil"
+	"github.com/snapcore/snapd/dirs"
+	. "gopkg.in/check.v1"
+)
+
+type sessionSuite struct{}
+
+var _ = Suite(&sessionSuite{})
+
+func (s *sessionSuite) SetUpTest(c *C) {
+	dirs.SetRootDir(c.MkDir())
+}
+
+func (s *sessionSuite) TestAvailableUserSessionsHappy(c *C) {
+	// fake two sockets, one for 0 and one for 1000
+	err := os.MkdirAll(path.Join(dirs.XdgRuntimeDirBase, "0", "snapd-session-agent.socket"), 0700)
+	c.Assert(err, IsNil)
+	err = os.MkdirAll(path.Join(dirs.XdgRuntimeDirBase, "1000", "snapd-session-agent.socket"), 0700)
+	c.Assert(err, IsNil)
+	err = os.MkdirAll(path.Join(dirs.XdgRuntimeDirBase, "1337", "snapd-session-agent.socket"), 0700)
+	c.Assert(err, IsNil)
+
+	res, err := clientutil.AvailableUserSessions()
+	c.Assert(err, IsNil)
+	c.Check(res, HasLen, 3)
+	sort.Ints(res)
+	c.Check(res, DeepEquals, []int{
+		0,
+		1000,
+		1337,
+	})
+}
+
+func (s *sessionSuite) TestAvailableUserSessionsIgnoresBadUids(c *C) {
+	// fake two sockets, one for 0 and one for 1000
+	err := os.MkdirAll(path.Join(dirs.XdgRuntimeDirBase, "hello", "snapd-session-agent.socket"), 0700)
+	c.Assert(err, IsNil)
+	err = os.MkdirAll(path.Join(dirs.XdgRuntimeDirBase, "*34i8932", "snapd-session-agent.socket"), 0700)
+	c.Assert(err, IsNil)
+	err = os.MkdirAll(path.Join(dirs.XdgRuntimeDirBase, "3", "snapd-session-agent.socket"), 0700)
+	c.Assert(err, IsNil)
+
+	res, err := clientutil.AvailableUserSessions()
+	c.Assert(err, IsNil)
+	c.Check(res, HasLen, 1)
+	sort.Ints(res)
+	c.Check(res, DeepEquals, []int{
+		3,
+	})
+}

overlord/servicestate/helpers.go

@@ -22,42 +22,19 @@ package servicestate
 import (
 	"fmt"
 	"os/user"
-	"path/filepath"
 	"sort"
 	"strconv"
 
-	"github.com/snapcore/snapd/dirs"
+	"github.com/snapcore/snapd/client/clientutil"
 	"github.com/snapcore/snapd/overlord/snapstate"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/wrappers"
 )
 
 var userLookup = user.Lookup
 
-// availableUsers returns a list of available valid user-session targets for
-// snapd.
-func availableUsers() ([]int, error) {
-	sockets, err := filepath.Glob(filepath.Join(dirs.XdgRuntimeDirGlob, "snapd-session-agent.socket"))
-	if err != nil {
-		return nil, err
-	}
-
-	var uids []int
-	for _, sock := range sockets {
-		uidStr := filepath.Base(filepath.Dir(sock))
-		uid, err := strconv.Atoi(uidStr)
-		if err != nil {
-			// Ignore directories that do not
-			// appear to be valid XDG runtime dirs
-			// (i.e. /run/user/NNNN).
-			continue
-		}
-		uids = append(uids, uid)
-	}
-	return uids, nil
-}
-
-// usernamesToUids converts a list of usernames, to a list of uids
+// usernamesToUids converts a list of usernames, to a list of uids by
+// looking up the usernames.
 func usernamesToUids(usernames []string) ([]int, error) {
 	uids := make([]int, 0, len(usernames))
 	for _, username := range usernames {
@@ -74,6 +51,25 @@ func usernamesToUids(usernames []string) ([]int, error) {
 	return uids, nil
 }
 
+func affectedUids(users []string) (map[int]bool, error) {
+	var uids []int
+	var err error
+	if len(users) == 0 {
+		uids, err = clientutil.AvailableUserSessions()
+	} else {
+		uids, err = usernamesToUids(users)
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	uidsAffected := make(map[int]bool, len(users))
+	for _, uid := range uids {
+		uidsAffected[uid] = true
+	}
+	return uidsAffected, nil
+}
+
 func splitServicesIntoSystemAndUser(apps []*snap.AppInfo) (sys, usr []*snap.AppInfo) {
 	for _, app := range apps {
 		if !app.IsService() {
@@ -146,23 +142,7 @@ func updateSnapstateSystemServices(snapst *snapstate.SnapState, apps []*snap.App
 	return true
 }
 
-func updateSnapstateUserServices(snapst *snapstate.SnapState, apps []*snap.AppInfo, enable bool, users []string) (bool, error) {
-	var uids []int
-	var err error
-	if len(users) == 0 {
-		uids, err = availableUsers()
-	} else {
-		uids, err = usernamesToUids(users)
-	}
-	if err != nil {
-		return false, nil
-	}
-
-	uidTargets := make(map[int]bool, len(users))
-	for _, uid := range uids {
-		uidTargets[uid] = true
-	}
-
+func updateSnapstateUserServices(snapst *snapstate.SnapState, apps []*snap.AppInfo, enable bool, uids map[int]bool) bool {
 	// populate helper lookups of already enabled/disabled services from
 	// snapst.
 	alreadyEnabled := make(map[int]map[string]bool)
@@ -187,7 +167,7 @@ func updateSnapstateUserServices(snapst *snapstate.SnapState, apps []*snap.AppIn
 		for _, service := range services {
 			for uid := range toState {
 				// otherwise migrate only if the user is a match
-				if !uidTargets[uid] {
+				if !uids[uid] {
 					continue
 				}
 
@@ -211,43 +191,44 @@ func updateSnapstateUserServices(snapst *snapstate.SnapState, apps []*snap.AppIn
 	}
 
 	// ensure uids are in target
-	for _, uid := range uids {
+	for uid := range uids {
 		if toState[uid] == nil {
 			toState[uid] = make(map[string]bool)
 		}
 	}
 
 	if changed := toggleServices(apps, fromState, toState); !changed {
 		// nothing changed
-		return false, nil
+		return false
 	}
 
-	// reset and recreate the state
-	snapst.UserServicesEnabledByHooks = nil
-	snapst.UserServicesDisabledByHooks = nil
-	if len(alreadyEnabled) != 0 {
-		snapst.UserServicesEnabledByHooks = make(map[int][]string, len(alreadyEnabled))
-		for uid, svcs := range alreadyEnabled {
+	convertStateMap := func(svcState map[int]map[string]bool) map[int][]string {
+		result := make(map[int][]string, len(svcState))
+		for uid, svcs := range svcState {
+			if len(svcs) == 0 {
+				continue
+			}
+
 			l := make([]string, 0, len(svcs))
 			for srv := range svcs {
 				l = append(l, srv)
 			}
 			sort.Strings(l)
-			snapst.UserServicesEnabledByHooks[uid] = l
+			result[uid] = l
 		}
+		return result
+	}
+
+	// reset and recreate the state
+	snapst.UserServicesEnabledByHooks = nil
+	snapst.UserServicesDisabledByHooks = nil
+	if len(alreadyEnabled) != 0 {
+		snapst.UserServicesEnabledByHooks = convertStateMap(alreadyEnabled)
 	}
 	if len(alreadyDisabled) != 0 {
-		snapst.UserServicesDisabledByHooks = make(map[int][]string, len(alreadyDisabled))
-		for uid, svcs := range alreadyDisabled {
-			l := make([]string, 0, len(svcs))
-			for srv := range svcs {
-				l = append(l, srv)
-			}
-			sort.Strings(l)
-			snapst.UserServicesDisabledByHooks[uid] = l
-		}
+		snapst.UserServicesDisabledByHooks = convertStateMap(alreadyDisabled)
 	}
-	return true, nil
+	return true
 }
 
 // updateSnapstateServices uses {User,}ServicesEnabledByHooks and {User,}ServicesDisabledByHooks in
@@ -271,21 +252,26 @@ func updateSnapstateServices(snapst *snapstate.SnapState, enable, disable []*sna
 		sys, usr = splitServicesIntoSystemAndUser(disable)
 	}
 
-	var sysChanged, usrChanged bool
 	isEnable := len(enable) > 0
 	switch scopeOpts.Scope {
 	case wrappers.ServiceScopeAll:
-		// Update user-services first as that one can error out
-		if changed, err := updateSnapstateUserServices(snapst, usr, isEnable, scopeOpts.Users); err != nil {
+		// Retrieve the uids affected
+		affectedUids, err := affectedUids(scopeOpts.Users)
+		if err != nil {
 			return false, err
-		} else {
-			usrChanged = changed
 		}
-		sysChanged = updateSnapstateSystemServices(snapst, sys, isEnable)
+		sysChanged := updateSnapstateSystemServices(snapst, sys, isEnable)
+		usrChanged := updateSnapstateUserServices(snapst, usr, isEnable, affectedUids)
+		return sysChanged || usrChanged, nil
 	case wrappers.ServiceScopeSystem:
-		sysChanged = updateSnapstateSystemServices(snapst, sys, isEnable)
+		return updateSnapstateSystemServices(snapst, sys, isEnable), nil
 	case wrappers.ServiceScopeUser:
-		return updateSnapstateUserServices(snapst, sys, isEnable, scopeOpts.Users)
+		// Retrieve the uids affected
+		affectedUids, err := affectedUids(scopeOpts.Users)
+		if err != nil {
+			return false, err
+		}
+		return updateSnapstateUserServices(snapst, sys, isEnable, affectedUids), nil
 	}
-	return sysChanged || usrChanged, nil
+	return false, nil
 }

overlord/servicestate/service_control_test.go

@@ -1404,7 +1404,6 @@ func (s *serviceControlSuite) TestUpdateSnapstateUserServices(c *C) {
 			disable: []string{"a"},
 			users:   []string{"root"},
 			expectedSnapstateEnabled: map[int][]string{
-				0:    {},
 				1000: {"a"},
 			},
 			expectedSnapstateDisabled: map[int][]string{
@@ -1420,10 +1419,8 @@ func (s *serviceControlSuite) TestUpdateSnapstateUserServices(c *C) {
 				0:    {"a", "c"},
 				1000: {"a"},
 			},
-			expectedSnapstateDisabled: map[int][]string{
-				0: {},
-			},
-			changed: true,
+			expectedSnapstateDisabled: map[int][]string{},
+			changed:                   true,
 		},
 		// We now mix it up with disabling for everyone
 		{

overlord/snapstate/backend_test.go

@@ -1206,8 +1206,13 @@ func (f *fakeSnappyBackend) StartServices(svcs []*snap.AppInfo, disabledSvcs *wr
 		services: services,
 	}
 	// only add the services to the op if there's something to add
-	if disabledSvcs != nil && len(disabledSvcs.SystemServices) != 0 {
-		op.disabledServices = disabledSvcs.SystemServices
+	if disabledSvcs != nil {
+		if len(disabledSvcs.SystemServices) != 0 {
+			op.disabledServices = disabledSvcs.SystemServices
+		}
+		if len(disabledSvcs.UserServices) != 0 {
+			op.disabledUserServices = disabledSvcs.UserServices
+		}
 	}
 	f.appendOp(&op)
 	return f.maybeErrForLastOp()