From: Simon Ruderich <simon@ruderich.org>
Date: Sun, 9 May 2021 08:10:56 +0000 (+0200)
Subject: Improve and add comments
X-Git-Url: https://ruderich.org/simon/gitweb/?a=commitdiff_plain;h=5d6cc7f14a4bacc36bf3a23cd735a75ad4a90f1d;p=safcm%2Fsafcm.git

Improve and add comments
---

diff --git a/cmd/safcm-remote/main.go b/cmd/safcm-remote/main.go
index e9ce1cd..ab0bfae 100644
--- a/cmd/safcm-remote/main.go
+++ b/cmd/safcm-remote/main.go
@@ -1,4 +1,4 @@
-// Helper copied to the remote hosts to run commands and deploy configuration
+// Helper copied to the remote host to run commands and deploy configuration
 
 // Copyright (C) 2021  Simon Ruderich
 //
diff --git a/cmd/safcm-remote/sync/files.go b/cmd/safcm-remote/sync/files.go
index f388606..119a353 100644
--- a/cmd/safcm-remote/sync/files.go
+++ b/cmd/safcm-remote/sync/files.go
@@ -152,8 +152,8 @@ reopen:
 		if change.Old.Mode.Type() == fs.ModeSymlink {
 			// Some BSD systems permit changing permissions of
 			// symlinks but ignore them on traversal. To keep it
-			// simple we don't support that and always use 0777 for
-			// symlink permissions (the value on GNU/Linux).
+			// simple we don't support that and always use 0777
+			// for symlink permissions (the value on GNU/Linux).
 			//
 			// TODO: Add proper support for symlinks on BSD
 			change.Old.Mode |= 0777
@@ -267,7 +267,7 @@ reopen:
 		}
 	}
 
-	// Directory: create new directory (also type change to directory)
+	// Directory: create new directory, also type change to directory
 	if file.Mode.IsDir() && (change.Created || changeType) {
 		debugf("creating directory")
 		err := os.Mkdir(file.Path, 0700)
@@ -373,7 +373,7 @@ reopen:
 			os.Remove(tmpPath)
 			return err
 		}
-		// Permissions are irrelevant for symlinks
+		// Permissions are irrelevant for symlinks (on most systems)
 
 	default:
 		panic(fmt.Sprintf("invalid file type %s", file.Mode))
diff --git a/cmd/safcm-remote/sync/sync_test.go b/cmd/safcm-remote/sync/sync_test.go
index 4c0f08d..6f8025a 100644
--- a/cmd/safcm-remote/sync/sync_test.go
+++ b/cmd/safcm-remote/sync/sync_test.go
@@ -31,6 +31,8 @@ import (
 	"ruderich.org/simon/safcm/cmd/safcm-remote/run"
 )
 
+// testRunner implements run.Runner to test commands without actually running
+// them.
 type testRunner struct {
 	t         *testing.T
 	expCmds   []*exec.Cmd
diff --git a/cmd/safcm/config/groups.go b/cmd/safcm/config/groups.go
index 5f3f679..a1dd83a 100644
--- a/cmd/safcm/config/groups.go
+++ b/cmd/safcm/config/groups.go
@@ -34,7 +34,7 @@ const (
 	GroupRemoveSuffix     = GroupSpecialSeparator + "remove"
 )
 
-// Keep in sync with sync_info.go:infoGroupDetectedRegexp
+// Keep in sync with cmd/safcm/sync_info.go:infoGroupDetectedRegexp
 var groupNameRegexp = regexp.MustCompile(`^[a-z0-9_-]+$`)
 
 func LoadGroups(cfg *Config, hosts *Hosts) (map[string][]string, error) {
@@ -86,6 +86,8 @@ func LoadGroups(cfg *Config, hosts *Hosts) (map[string][]string, error) {
 			if x == GroupAll {
 				continue
 			}
+			// Don't validate against groupNameRegexp because
+			// hosts have less strict restrictions.
 			if strings.Contains(x, GroupSpecialSeparator) {
 				return nil, fmt.Errorf(
 					"%s member %q must not contain %q",
diff --git a/cmd/safcm/sync.go b/cmd/safcm/sync.go
index 98b118e..77e83c4 100644
--- a/cmd/safcm/sync.go
+++ b/cmd/safcm/sync.go
@@ -247,8 +247,8 @@ are only available after the hosts were contacted.
 		nameMap[x] = true
 	}
 	nameMatched := make(map[string]bool)
-	// To detect typos we must check all given names but only want to add
-	// each match once
+	// To detect typos we must check all given names but one host can be
+	// matched by multiple names (e.g. two groups with overlapping hosts)
 	hostMatched := make(map[string]bool)
 
 	var res []*config.Host
diff --git a/cmd/safcm/sync_info.go b/cmd/safcm/sync_info.go
index b0a85ee..0df33e3 100644
--- a/cmd/safcm/sync_info.go
+++ b/cmd/safcm/sync_info.go
@@ -45,7 +45,7 @@ func (s *Sync) hostInfo(conn *rpc.Conn) ([]string, error) {
 	return hostInfoRespToGroups(resp), nil
 }
 
-// Keep in sync with config/groups.go:groupNameRegexp
+// Keep in sync with cmd/safcm/config/groups.go:groupNameRegexp
 var infoGroupDetectedRegexp = regexp.MustCompile(`[^a-z0-9_-]+`)
 
 func hostInfoRespToGroups(resp safcm.MsgInfoResp) []string {
diff --git a/cmd/safcm/sync_sync.go b/cmd/safcm/sync_sync.go
index 6977edd..88c93be 100644
--- a/cmd/safcm/sync_sync.go
+++ b/cmd/safcm/sync_sync.go
@@ -186,7 +186,7 @@ func (s *Sync) resolveHostGroups(detectedGroups []string) (
 		return nil, nil, err
 	}
 
-	// Early entries have higher priorities
+	// Early entries in "group_priority" have higher priorities
 	groupPriority := make(map[string]int)
 	for i, x := range s.config.GroupPriority {
 		groupPriority[x] = len(s.config.GroupPriority) - i
diff --git a/cmd/safcm/term.go b/cmd/safcm/term.go
index d7568e4..47f7d62 100644
--- a/cmd/safcm/term.go
+++ b/cmd/safcm/term.go
@@ -57,7 +57,8 @@ func ColorString(isTTY bool, color Color, x string) string {
 var escapeRegexp = regexp.MustCompile(`[\x00-\x08\x0B-\x1F\x7F]`)
 
 // EscapeControlCharacters escapes all ASCII control characters (except
-// newline and tab) by replacing them with their hex value.
+// newline and tab) by replacing them with their hex value. If the output is
+// to a TTY then the escaped characters are colored.
 //
 // This function must be used when displaying any input from remote hosts to
 // prevent terminal escape code injections.
diff --git a/rpc/conn.go b/rpc/conn.go
index 626a507..9786941 100644
--- a/rpc/conn.go
+++ b/rpc/conn.go
@@ -55,7 +55,7 @@ type ConnEvent struct {
 }
 
 // NewConn creates a new connection. Events in the returned struct must be
-// regularly read or the connection will stall. This must be done before
+// regularly read or the connection will hang. This must be done before
 // DialSSH is called to open a connection.
 func NewConn(debug bool) *Conn {
 	ch := make(chan ConnEvent)
diff --git a/rpc/dial.go b/rpc/dial.go
index 945a75f..0782ab0 100644
--- a/rpc/dial.go
+++ b/rpc/dial.go
@@ -141,13 +141,13 @@ compat_sha512sum() {
 	//
 	// The target directory must no permit other users to delete our files
 	// or symlink attacks and arbitrary code execution is possible. For
-	// /tmp this is guaranteed by the sticky bit. Make sure it has the
-	// proper permissions.
+	// /tmp this is guaranteed by the sticky bit. The code verifies the
+	// directory has the proper permissions.
 	//
 	// We cannot use `test -f && test -O` because this is open to TOCTOU
 	// attacks. `stat` gives use the full file state. If the file is owned
-	// by us and not a symlink then it's safe to use (assuming sticky or
-	// directory not writable by others).
+	// by us and not a symlink then it's safe to use (assuming sticky
+	// directory or directory not writable by others).
 	//
 	// `test -e` is only used to prevent error messages if the file
 	// doesn't exist. It does not guard against any races.
@@ -264,7 +264,7 @@ f
 				c.sshRemote,
 				fmt.Sprintf("cat > %q", path))...)
 		cmd.Stdin = bytes.NewReader(helper)
-		err = c.handleStderrAsEvents(cmd)
+		err = c.handleStderrAsEvents(cmd) // cmd.Stderr
 		if err != nil {
 			return err
 		}