services/device/device: make glob flags per-subcommand instead of global

The main motivation is to customize the default values of the glob flags
depending on the subcommand. For example, update wants to group parallelism by
kind, whereas status is happy to run with full parallelism; also (to come)
update should by default only care about active installations and non-deleted
instances.

Most of the changes are in glob.go, since I tried to package up the flag logic
in one place (to keep code in the specific subcommand files minimal).

Change-Id: I2ec04bd46a3e4ef80f53fe1ea7ef8b3d1ea9032e
diff --git a/services/device/device/debug.go b/services/device/device/debug.go
index 1ba6fc4..1b06b3c 100644
--- a/services/device/device/debug.go
+++ b/services/device/device/debug.go
@@ -15,7 +15,6 @@
 )
 
 var cmdDebug = &cmdline.Command{
-	Runner:   globRunner(runDebug),
 	Name:     "debug",
 	Short:    "Debug the device.",
 	Long:     "Get internal debug information about application installations and instances.",
@@ -24,6 +23,10 @@
 <app name patterns...> are vanadium object names or glob name patterns corresponding to application installations and instances.`,
 }
 
+func init() {
+	globify(cmdDebug, runDebug, new(globSettings))
+}
+
 func runDebug(entry globResult, ctx *context.T, stdout, _ io.Writer) error {
 	if description, err := device.DeviceClient(entry.name).Debug(ctx); err != nil {
 		return fmt.Errorf("Debug failed: %v", err)
diff --git a/services/device/device/doc.go b/services/device/device/doc.go
index aedb75e..16f457c 100644
--- a/services/device/device/doc.go
+++ b/services/device/device/doc.go
@@ -46,14 +46,6 @@
    user specified by this flag
  -dryrun=false
    Elides root-requiring systemcalls.
- -installation-state=
-   If non-empty, specifies allowed installation states (all others installations
-   get filtered out). The value of the flag is a comma-separated list of values
-   from among: [Active Uninstalled].
- -instance-state=
-   If non-empty, specifies allowed instance states (all other instances get
-   filtered out). The value of the flag is a comma-separated list of values from
-   among: [Launching Running Dying NotRunning Updating Deleted].
  -kill=false
    Kill process ids given as command-line arguments.
  -log_backtrace_at=:0
@@ -68,13 +60,6 @@
    max size in bytes of the buffer to use for logging stack traces
  -minuid=501
    UIDs cannot be less than this number.
- -only-installations=false
-   If set, only consider installations.
- -only-instances=false
-   If set, only consider instances.
- -parallelism=BYKIND
-   Specifies the level of parallelism for the handler execution. One of:
-   BYKIND,FULL,NONE
  -progname=unnamed_app
    Visible name of the application, used in argv[0]
  -rm=false
@@ -287,41 +272,109 @@
 previous version of their current version
 
 Usage:
-   device revert <name patterns...>
+   device revert [flags] <name patterns...>
 
 <name patterns...> are vanadium object names or glob name patterns corresponding
 to the device manager service, or to application installations and instances.
 
+The device revert flags are:
+ -installation-state=
+   If non-empty, specifies allowed installation states (all others installations
+   get filtered out). The value of the flag is a comma-separated list of values
+   from among: [Active Uninstalled].
+ -instance-state=
+   If non-empty, specifies allowed instance states (all other instances get
+   filtered out). The value of the flag is a comma-separated list of values from
+   among: [Launching Running Dying NotRunning Updating Deleted].
+ -only-installations=false
+   If set, only consider installations.
+ -only-instances=false
+   If set, only consider instances.
+ -parallelism=BYKIND
+   Specifies the level of parallelism for the handler execution. One of: [BYKIND
+   FULL NONE].
+
 Device update
 
 Update the device manager or application instances and installations
 
 Usage:
-   device update <name patterns...>
+   device update [flags] <name patterns...>
 
 <name patterns...> are vanadium object names or glob name patterns corresponding
 to the device manager service, or to application installations and instances.
 
+The device update flags are:
+ -installation-state=
+   If non-empty, specifies allowed installation states (all others installations
+   get filtered out). The value of the flag is a comma-separated list of values
+   from among: [Active Uninstalled].
+ -instance-state=
+   If non-empty, specifies allowed instance states (all other instances get
+   filtered out). The value of the flag is a comma-separated list of values from
+   among: [Launching Running Dying NotRunning Updating Deleted].
+ -only-installations=false
+   If set, only consider installations.
+ -only-instances=false
+   If set, only consider instances.
+ -parallelism=BYKIND
+   Specifies the level of parallelism for the handler execution. One of: [BYKIND
+   FULL NONE].
+
 Device status
 
 Get the status of the device manager or application instances and installations.
 
 Usage:
-   device status <name patterns...>
+   device status [flags] <name patterns...>
 
 <name patterns...> are vanadium object names or glob name patterns corresponding
 to the device manager service, or to application installations and instances.
 
+The device status flags are:
+ -installation-state=
+   If non-empty, specifies allowed installation states (all others installations
+   get filtered out). The value of the flag is a comma-separated list of values
+   from among: [Active Uninstalled].
+ -instance-state=
+   If non-empty, specifies allowed instance states (all other instances get
+   filtered out). The value of the flag is a comma-separated list of values from
+   among: [Launching Running Dying NotRunning Updating Deleted].
+ -only-installations=false
+   If set, only consider installations.
+ -only-instances=false
+   If set, only consider instances.
+ -parallelism=FULL
+   Specifies the level of parallelism for the handler execution. One of: [BYKIND
+   FULL NONE].
+
 Device debug
 
 Get internal debug information about application installations and instances.
 
 Usage:
-   device debug <app name patterns...>
+   device debug [flags] <app name patterns...>
 
 <app name patterns...> are vanadium object names or glob name patterns
 corresponding to application installations and instances.
 
+The device debug flags are:
+ -installation-state=
+   If non-empty, specifies allowed installation states (all others installations
+   get filtered out). The value of the flag is a comma-separated list of values
+   from among: [Active Uninstalled].
+ -instance-state=
+   If non-empty, specifies allowed instance states (all other instances get
+   filtered out). The value of the flag is a comma-separated list of values from
+   among: [Launching Running Dying NotRunning Updating Deleted].
+ -only-installations=false
+   If set, only consider installations.
+ -only-instances=false
+   If set, only consider instances.
+ -parallelism=FULL
+   Specifies the level of parallelism for the handler execution. One of: [BYKIND
+   FULL NONE].
+
 Device acl - Tool for setting device manager Permissions
 
 The acl tool manages Permissions on the device manger, installations and
@@ -404,11 +457,28 @@
 List application installations or instances.
 
 Usage:
-   device ls <app name patterns...>
+   device ls [flags] <app name patterns...>
 
 <app name patterns...> are vanadium object names or glob name patterns
 corresponding to application installations and instances.
 
+The device ls flags are:
+ -installation-state=
+   If non-empty, specifies allowed installation states (all others installations
+   get filtered out). The value of the flag is a comma-separated list of values
+   from among: [Active Uninstalled].
+ -instance-state=
+   If non-empty, specifies allowed instance states (all other instances get
+   filtered out). The value of the flag is a comma-separated list of values from
+   among: [Launching Running Dying NotRunning Updating Deleted].
+ -only-installations=false
+   If set, only consider installations.
+ -only-instances=false
+   If set, only consider instances.
+ -parallelism=FULL
+   Specifies the level of parallelism for the handler execution. One of: [BYKIND
+   FULL NONE].
+
 Device help - Display help for commands or topics
 
 Help with no args displays the usage of the parent command.
diff --git a/services/device/device/glob.go b/services/device/device/glob.go
index 63a0e4c..523e3ec 100644
--- a/services/device/device/glob.go
+++ b/services/device/device/glob.go
@@ -33,7 +33,10 @@
 // and can be run concurrently. The handler should direct its output to the
 // given stdout and stderr writers.
 //
-// Typical usage:
+// There are three usage patterns, depending on the desired level of control
+// over the execution flow and settings manipulation.
+//
+// (1) Most control
 //
 // func myCmdHandler(entry globResult, ctx *context.T, stdout, stderr io.Writer) error {
 //   output := myCmdProcessing(entry)
@@ -43,7 +46,7 @@
 //
 // func runMyCmd(ctx *context.T, env *cmdline.Env, args []string) error {
 //   ...
-//   err := run(ctx, env, args, myCmdHandler)
+//   err := run(ctx, env, args, myCmdHandler, globSettings{})
 //   ...
 // }
 //
@@ -52,18 +55,35 @@
 //   ...
 // }
 //
-// Alternatively, if all runMyCmd does is to call run, you can use globRunner to
-// avoid having to define runMyCmd:
+// (2) Pre-packaged runner
+//
+// If all runMyCmd does is to call run, you can use globRunner to avoid having
+// to define runMyCmd:
 //
 // var cmdMyCmd = &cmdline.Command {
-//   Runner: globRunner(myCmdHandler)
+//   Runner: globRunner(myCmdHandler, &globSettings{}),
+//   Name: "mycmd",
 //   ...
 // }
+//
+// (3) Pre-packaged runner and glob settings flag configuration
+//
+// If, additionally, you want the globSettings to be configurable with
+// command-line flags, you can use globify instead:
+//
+// var cmdMyCmd = &cmdline.Command {
+//   Name: "mycmd",
+//   ...
+// }
+//
+// func init() {
+//   globify(cmdMyCmd, myCmdHandler, &globSettings{}),
+// }
 type globHandler func(entry globResult, ctx *context.T, stdout, stderr io.Writer) error
 
-func globRunner(handler globHandler) cmdline.Runner {
+func globRunner(handler globHandler, s *globSettings) cmdline.Runner {
 	return v23cmd.RunnerFunc(func(ctx *context.T, env *cmdline.Env, args []string) error {
-		return run(ctx, env, args, handler)
+		return run(ctx, env, args, handler, *s)
 	})
 }
 
@@ -113,10 +133,10 @@
 // (instance/installation) and state.  No de-duping of results is performed.
 // The outputs from each of the handlers are sorted: installations first, then
 // instances (and alphabetically by object name for each group).
-func run(ctx *context.T, env *cmdline.Env, args []string, handler globHandler) error {
+func run(ctx *context.T, env *cmdline.Env, args []string, handler globHandler, s globSettings) error {
 	results := glob(ctx, env, args)
 	sort.Sort(byTypeAndName(results))
-	results = filterResults(results)
+	results = filterResults(results, s)
 	stdouts, stderrs := make([]bytes.Buffer, len(results)), make([]bytes.Buffer, len(results))
 	var errorCounter uint32 = 0
 	perResult := func(r globResult, index int) {
@@ -126,7 +146,7 @@
 		}
 	}
 	// TODO(caprita): Add unit test logic to cover all parallelism options.
-	switch handlerParallelism {
+	switch s.handlerParallelism {
 	case fullParallelism:
 		var wg sync.WaitGroup
 		for i, r := range results {
@@ -172,16 +192,16 @@
 	return nil
 }
 
-func filterResults(results []globResult) []globResult {
+func filterResults(results []globResult, s globSettings) []globResult {
 	var ret []globResult
 	for _, r := range results {
-		switch s := r.status.(type) {
+		switch status := r.status.(type) {
 		case device.StatusInstance:
-			if onlyInstallations || !instanceStateFilter.apply(s.Value.State) {
+			if s.onlyInstallations || !s.instanceStateFilter.apply(status.Value.State) {
 				continue
 			}
 		case device.StatusInstallation:
-			if onlyInstances || !installationStateFilter.apply(s.Value.State) {
+			if s.onlyInstances || !s.installationStateFilter.apply(status.Value.State) {
 				continue
 			}
 		}
@@ -361,8 +381,6 @@
 	kindParallelism: "BYKIND",
 }
 
-const defaultParallelism = kindParallelism
-
 func init() {
 	if len(parallelismStrings) != int(sentinelParallelismFlag) {
 		panic(fmt.Sprintf("broken invariant: mismatching number of parallelism types"))
@@ -387,35 +405,52 @@
 	return fmt.Errorf("unrecognized parallelism type: %v", s)
 }
 
-var (
+type globSettings struct {
 	instanceStateFilter     instanceStateFlag
 	installationStateFilter installationStateFlag
 	onlyInstances           bool
 	onlyInstallations       bool
-	handlerParallelism      parallelismFlag = defaultParallelism
-)
+	handlerParallelism      parallelismFlag
+	defaults                *globSettings
+}
 
-func init() {
-	// NOTE: When addind new flags or changing default values, remember to
-	// also update ResetGlobFlags below.
-	flag.Var(&instanceStateFilter, "instance-state", fmt.Sprintf("If non-empty, specifies allowed instance states (all other instances get filtered out). The value of the flag is a comma-separated list of values from among: %v.", device.InstanceStateAll))
-	flag.Var(&installationStateFilter, "installation-state", fmt.Sprintf("If non-empty, specifies allowed installation states (all others installations get filtered out). The value of the flag is a comma-separated list of values from among: %v.", device.InstallationStateAll))
-	flag.BoolVar(&onlyInstances, "only-instances", false, "If set, only consider instances.")
-	flag.BoolVar(&onlyInstallations, "only-installations", false, "If set, only consider installations.")
+func (s *globSettings) reset() {
+	d := s.defaults
+	*s = *d
+	s.defaults = d
+}
+
+func (s *globSettings) setDefaults(d globSettings) {
+	s.defaults = new(globSettings)
+	*s.defaults = d
+}
+
+var allGlobSettings []*globSettings
+
+// ResetGlobSettings is meant for tests to restore the values of flag-configured
+// variables when running multiple commands in the same process.
+func ResetGlobSettings() {
+	for _, s := range allGlobSettings {
+		s.reset()
+	}
+}
+
+func defineGlobFlags(fs *flag.FlagSet, s *globSettings) {
+	fs.Var(&s.instanceStateFilter, "instance-state", fmt.Sprintf("If non-empty, specifies allowed instance states (all other instances get filtered out). The value of the flag is a comma-separated list of values from among: %v.", device.InstanceStateAll))
+	fs.Var(&s.installationStateFilter, "installation-state", fmt.Sprintf("If non-empty, specifies allowed installation states (all others installations get filtered out). The value of the flag is a comma-separated list of values from among: %v.", device.InstallationStateAll))
+	fs.BoolVar(&s.onlyInstances, "only-instances", false, "If set, only consider instances.")
+	fs.BoolVar(&s.onlyInstallations, "only-installations", false, "If set, only consider installations.")
 	var parallelismValues []string
 	for _, v := range parallelismStrings {
 		parallelismValues = append(parallelismValues, v)
 	}
 	sort.Strings(parallelismValues)
-	flag.Var(&handlerParallelism, "parallelism", "Specifies the level of parallelism for the handler execution. One of: "+strings.Join(parallelismValues, ","))
+	fs.Var(&s.handlerParallelism, "parallelism", fmt.Sprintf("Specifies the level of parallelism for the handler execution. One of: %v.", parallelismValues))
 }
 
-// ResetGlobFlags is meant for tests to restore the values of flag-configured
-// variables when running multiple commands in the same process.
-func ResetGlobFlags() {
-	instanceStateFilter = make(instanceStateFlag)
-	installationStateFilter = make(installationStateFlag)
-	onlyInstances = false
-	onlyInstallations = false
-	handlerParallelism = defaultParallelism
+func globify(c *cmdline.Command, handler globHandler, s *globSettings) {
+	s.setDefaults(*s)
+	defineGlobFlags(&c.Flags, s)
+	c.Runner = globRunner(handler, s)
+	allGlobSettings = append(allGlobSettings, s)
 }
diff --git a/services/device/device/ls.go b/services/device/device/ls.go
index 80939da..9767abc 100644
--- a/services/device/device/ls.go
+++ b/services/device/device/ls.go
@@ -14,7 +14,6 @@
 )
 
 var cmdLs = &cmdline.Command{
-	Runner:   globRunner(runLs),
 	Name:     "ls",
 	Short:    "List applications.",
 	Long:     "List application installations or instances.",
@@ -23,6 +22,10 @@
 <app name patterns...> are vanadium object names or glob name patterns corresponding to application installations and instances.`,
 }
 
+func init() {
+	globify(cmdLs, runLs, new(globSettings))
+}
+
 func runLs(entry globResult, _ *context.T, stdout, _ io.Writer) error {
 	fmt.Fprintf(stdout, "%v\n", entry.name)
 	return nil
diff --git a/services/device/device/ls_test.go b/services/device/device/ls_test.go
index 420dd05..1db54f4 100644
--- a/services/device/device/ls_test.go
+++ b/services/device/device/ls_test.go
@@ -159,6 +159,6 @@
 		if expected, got := c.expected, strings.TrimSpace(stdout.String()); got != expected {
 			t.Errorf("Unexpected output from ls. Got %q, expected %q", got, expected)
 		}
-		cmd_device.ResetGlobFlags()
+		cmd_device.ResetGlobSettings()
 	}
 }
diff --git a/services/device/device/status.go b/services/device/device/status.go
index d32430e..6123ec1 100644
--- a/services/device/device/status.go
+++ b/services/device/device/status.go
@@ -14,7 +14,6 @@
 )
 
 var cmdStatus = &cmdline.Command{
-	Runner:   globRunner(runStatus),
 	Name:     "status",
 	Short:    "Get device manager or application status.",
 	Long:     "Get the status of the device manager or application instances and installations.",
@@ -23,6 +22,10 @@
 <name patterns...> are vanadium object names or glob name patterns corresponding to the device manager service, or to application installations and instances.`,
 }
 
+func init() {
+	globify(cmdStatus, runStatus, new(globSettings))
+}
+
 func runStatus(entry globResult, _ *context.T, stdout, _ io.Writer) error {
 	switch s := entry.status.(type) {
 	case device.StatusInstance:
diff --git a/services/device/device/status_test.go b/services/device/device/status_test.go
index 4284226..0f2d8e0 100644
--- a/services/device/device/status_test.go
+++ b/services/device/device/status_test.go
@@ -69,5 +69,6 @@
 		if got, expected := appTape.Play(), []interface{}{"Status"}; !reflect.DeepEqual(expected, got) {
 			t.Errorf("invalid call sequence. Got %v, want %v", got, expected)
 		}
+		cmd_device.ResetGlobSettings()
 	}
 }
diff --git a/services/device/device/update.go b/services/device/device/update.go
index e2d1f9a..33d7ecd 100644
--- a/services/device/device/update.go
+++ b/services/device/device/update.go
@@ -20,7 +20,6 @@
 )
 
 var cmdUpdate = &cmdline.Command{
-	Runner:   globRunner(runUpdate),
 	Name:     "update",
 	Short:    "Update the device manager or applications.",
 	Long:     "Update the device manager or application instances and installations",
@@ -29,8 +28,11 @@
 <name patterns...> are vanadium object names or glob name patterns corresponding to the device manager service, or to application installations and instances.`,
 }
 
+func init() {
+	globify(cmdUpdate, runUpdate, &globSettings{handlerParallelism: kindParallelism})
+}
+
 var cmdRevert = &cmdline.Command{
-	Runner:   globRunner(runRevert),
 	Name:     "revert",
 	Short:    "Revert the device manager or applications.",
 	Long:     "Revert the device manager or application instances and installations to a previous version of their current version",
@@ -39,6 +41,10 @@
 <name patterns...> are vanadium object names or glob name patterns corresponding to the device manager service, or to application installations and instances.`,
 }
 
+func init() {
+	globify(cmdRevert, runRevert, &globSettings{handlerParallelism: kindParallelism})
+}
+
 func instanceIsRunning(ctx *context.T, von string) (bool, error) {
 	status, err := device.ApplicationClient(von).Status(ctx)
 	if err != nil {
diff --git a/services/device/device/update_test.go b/services/device/device/update_test.go
index a6f0a40..07f7616 100644
--- a/services/device/device/update_test.go
+++ b/services/device/device/update_test.go
@@ -145,7 +145,7 @@
 					t.Errorf("Unexpected stimuli for %v. Want: %v, got %v.", n, want, got)
 				}
 			}
-			cmd_device.ResetGlobFlags()
+			cmd_device.ResetGlobSettings()
 		}
 	}
 }