base: refactor LaunchApp variants into a single function

base/process_util.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This file/namespace contains utility functions for enumerating, ending and
 // computing statistics of processes.
 
 #ifndef BASE_PROCESS_UTIL_H_
 #define BASE_PROCESS_UTIL_H_
 #pragma once
@@ skipping 172 matching lines @@
 #if defined(OS_POSIX)
 // Returns the ID for the parent of the given process.
 BASE_API ProcessId GetParentProcessId(ProcessHandle process);
 
 // Close all file descriptors, except those which are a destination in the
 // given multimap. Only call this function in a child process where you know
 // that there aren't any other threads.
 BASE_API void CloseSuperfluousFds(const InjectiveMultimap& saved_map);
 #endif
 
+// TODO(evan): rename these to use StudlyCaps.
+typedef std::vector<std::pair<std::string, std::string> > environment_vector;
+typedef std::vector<std::pair<int, int> > file_handle_mapping_vector;
+
+struct LaunchOptions {
+  // If true, wait for the process to complete.
+  bool wait;
+
+  // If non-NULL, will be filled in with the handle of the lauched process.
+  // NOTE: In this case, the caller is responsible for closing the handle so
+  //       that it doesn't leak!

[Mark Mentovai, 2011/06/29 22:11:28]

And if NULL, what happens? Does the launcher wait for it, or does it leak
regardless? Not clear based on the comment.

[Evan Martin, 2011/06/29 23:09:26]

Copy'n'paste from the previous code.  Will clarify.

+  ProcessHandle* process_handle;
+
+#if defined(OS_WIN)
+  bool start_hidden;
+
+  // If true, the new process inherits handles from the parent.
+  bool inherit_handles;
+
+  // If non-NULL, runs as if the user represented by the token had launched it.
+  // Whether the application is visible on the interactive desktop depends on
+  // the token belonging to an interactive logon session.
+  //
+  // To avoid hard to diagnose problems, when specified this loads the
+  // environment variables associated with the user and if this operation fails
+  // the entire call fails as well.
+  UserTokenHandle as_user;
+
+  // If true, use an empty string for the desktop name.
+  bool empty_desktop_name;
+#else
+  // If non-NULL, set/unset environment variables.

[Mark Mentovai, 2011/06/29 22:11:28]

What does it mean to “unset”? How would you “unset?” I thought “environ” would
just be used as a replacement environment.

[Evan Martin, 2011/06/29 23:09:26]

To be honest, I didn't fully understand it myself.  Updated with a comment
(which points down to ReplaceEnv).

+  const environment_vector* environ;
+
+  // If non-NULL, remap file descriptors according to the mapping of
+  // src fd->dest fd to propagate FDs into the child process.
+  const file_handle_mapping_vector* fds_to_remap;

[Mark Mentovai, 2011/06/29 22:11:28]

Seems like this can just be a file_handle_mapping_vector, no *.

I may have a similar comment for |environ| depending on what you say.

[Evan Martin, 2011/06/29 23:09:26]

Hm, yeah.  When I first wrote this I was designing it such that you could
memset() the whole struct to zero to get the default values.  But now that I've
typed that out I have no idea why I thought that was desirable; clearly I've
been writing too much Go code lately.  :(

...edit: oh, there actually is a good reason -- the callers of the old LaunchApp
functions pass in a const ref to these objects, and I want to pass them along
without making a copy of them into this struct.  Do you think that's premature
optimization?

+
+  // If true, start the process in a new process group, instead of
+  // inheritng the parent's process group.  The pgid of the child process
+  // will be the same as its pid.
+  bool new_process_group;
+#endif
+
+  LaunchOptions() : wait(false), process_handle(NULL),

[Mark Mentovai, 2011/06/29 22:11:28]

Can you move the constructor up to the top as is ordinary in our code?

+#if defined(OS_WIN)
+                    start_hidden(false), inherit_handles(false), as_user(NULL),
+                    empty_desktop_name(false)
+#else
+                    environ(NULL), fds_to_remap(NULL), new_process_group(false)
+#endif
+      {}
+};
+
+// Launch a process via the command line |cmdline|.
+// See the documentation of LaunchOptions for details on launching options.
+//
+// Unix-specific notes:
+// - Before launching all FDs open in the parent process will be marked as

[Mark Mentovai, 2011/06/29 22:11:28]

launching comma all. Because the first time I read this, I thought you were
going to launch the FDs.

+//   close-on-exec.
+// - If the first argument on the command line is not fully specified,

[Mark Mentovai, 2011/06/29 22:11:28]

“fully specified” means what, exactly? No slashes in it? Absolute path?

[Evan Martin, 2011/06/29 23:09:26]

Copy'n'paste from the previous code.  Will clarify.

+//   PATH will be searched.
+BASE_API bool LaunchProcess(const CommandLine& cmdline,
+                            const LaunchOptions& options);
+
 #if defined(OS_WIN)
 
 enum IntegrityLevel {
   INTEGRITY_UNKNOWN,
   LOW_INTEGRITY,
   MEDIUM_INTEGRITY,
   HIGH_INTEGRITY,
 };
 // Determine the integrity level of the specified process. Returns false
 // if the system does not support integrity levels (pre-Vista) or in the case
 // of an underlying system failure.
 BASE_API bool GetProcessIntegrityLevel(ProcessHandle process,
                                        IntegrityLevel *level);
 
-// Runs the given application name with the given command line. Normally, the
-// first command line argument should be the path to the process, and don't
-// forget to quote it.
+// Windows-specific LaunchProcess that takes the command line as a string.
+// Prefer using the CommandLine LaunchProcess if possible.
 //
-// If wait is true, it will block and wait for the other process to finish,
-// otherwise, it will just continue asynchronously.
+// The first command line argument should be the path to the process,
+// and don't forget to quote it.
 //
 // Example (including literal quotes)
 //  cmdline = "c:\windows\explorer.exe" -foo "c:\bar\"
-//
-// If process_handle is non-NULL, the process handle of the launched app will be
-// stored there on a successful launch.
-// NOTE: In this case, the caller is responsible for closing the handle so
-//       that it doesn't leak!
-BASE_API bool LaunchApp(const std::wstring& cmdline,
-                        bool wait, bool start_hidden,
-                        ProcessHandle* process_handle);
+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
+BASE_API bool LaunchProcess(const string16& cmdline,
+                            const LaunchOptions& options);
 
-// Same as LaunchApp, except allows the new process to inherit handles of the
-// parent process.
-BASE_API bool LaunchAppWithHandleInheritance(const std::wstring& cmdline,
-                                             bool wait, bool start_hidden,
-                                             ProcessHandle* process_handle);
+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
+inline bool LaunchApp(const std::wstring& cmdline,
+                      bool wait, bool start_hidden,
+                      ProcessHandle* process_handle) {
+  LaunchOptions options;
+  options.wait = wait;
+  options.start_hidden = start_hidden;
+  options.process_handle = process_handle;
+  return LaunchProcess(cmdline, options);
+}
 
-// Runs the given application name with the given command line as if the user
-// represented by |token| had launched it. The caveats about |cmdline| and
-// |process_handle| explained for LaunchApp above apply as well.
-//
-// Whether the application is visible on the interactive desktop depends on
-// the token belonging to an interactive logon session.
-//
-// To avoid hard to diagnose problems, this function internally loads the
-// environment variables associated with the user and if this operation fails
-// the entire call fails as well.
-BASE_API bool LaunchAppAsUser(UserTokenHandle token,
-                              const std::wstring& cmdline,
-                              bool start_hidden,
-                              ProcessHandle* process_handle);
+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
+inline bool LaunchAppWithHandleInheritance(const std::wstring& cmdline,
+                                           bool wait, bool start_hidden,
+                                           ProcessHandle* process_handle) {
+  LaunchOptions options;
+  options.wait = wait;
+  options.start_hidden = start_hidden;
+  options.process_handle = process_handle;
+  options.inherit_handles = true;
+  return LaunchProcess(cmdline, options);
+}
 
-// Has the same behavior as LaunchAppAsUser, but offers the boolean option to
-// use an empty string for the desktop name and a boolean for allowing the
-// child process to inherit handles from its parent.
-BASE_API bool LaunchAppAsUser(UserTokenHandle token,
-                              const std::wstring& cmdline,
-                              bool start_hidden, ProcessHandle* process_handle,
-                              bool empty_desktop_name, bool inherit_handles);
+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
+inline bool LaunchAppAsUser(UserTokenHandle token,
+                            const std::wstring& cmdline,
+                            bool start_hidden,
+                            ProcessHandle* process_handle) {
+  LaunchOptions options;
+  options.start_hidden = start_hidden;
+  options.process_handle = process_handle;
+  options.as_user = token;
+  return LaunchProcess(cmdline, options);
+}
 
+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
+inline bool LaunchAppAsUser(UserTokenHandle token,
+                            const std::wstring& cmdline,
+                            bool start_hidden, ProcessHandle* process_handle,
+                            bool empty_desktop_name, bool inherit_handles) {
+  LaunchOptions options;
+  options.start_hidden = start_hidden;
+  options.process_handle = process_handle;
+  options.as_user = token;
+  options.empty_desktop_name = empty_desktop_name;
+  options.inherit_handles = inherit_handles;
+  return LaunchProcess(cmdline, options);
+}
 
 #elif defined(OS_POSIX)
-// Runs the application specified in argv[0] with the command line argv.
-// Before launching all FDs open in the parent process will be marked as
-// close-on-exec.  |fds_to_remap| defines a mapping of src fd->dest fd to
-// propagate FDs into the child process.
-//
-// As above, if wait is true, execute synchronously. The pid will be stored
-// in process_handle if that pointer is non-null.
-//
-// Note that the first argument in argv must point to the executable filename.
-// If the filename is not fully specified, PATH will be searched.
-typedef std::vector<std::pair<int, int> > file_handle_mapping_vector;
-BASE_API bool LaunchApp(const std::vector<std::string>& argv,
-                        const file_handle_mapping_vector& fds_to_remap,
-                        bool wait, ProcessHandle* process_handle);
+// A POSIX-specific version of LaunchProcess that takes an argv array
+// instead of a CommandLine.  Prefer the CommandLine version if possible.

[Mark Mentovai, 2011/06/29 22:11:28]

Doesn’t the CommandLine version F the argument order? I hate that. I hate it
enough to not encourage its use unless you already have a CommandLine in your
hands. Same comment applies to the Windows thing above.

I hit this last week. Using CommandLine, all of my “loose” arguments got moved
to the end of the vector. That’s OK for how Chromium parses arguments, but not
everything we launch (or will launch) is Chromium. It was scary enough to turn
me off of the CommandLine stuff (again).

Be careful in this change and subsequent cleanups that you don’t force anyone
who was using a vector of strings into the CommandLine path.

[Evan Martin, 2011/06/29 23:09:26]

I think CommandLine() is only useful for launching Chrome itself, since it also
contains our own assumptions about how arguments (particularly switches) will be
parsed.  I will update the comment.

+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
+BASE_API bool LaunchProcess(const std::vector<std::string>& argv,
+                            const LaunchOptions& options);
 
-// Similar to the above, but also (un)set environment variables in child process
-// through |environ|.
-typedef std::vector<std::pair<std::string, std::string> > environment_vector;
-BASE_API bool LaunchApp(const std::vector<std::string>& argv,
-                        const environment_vector& environ,
-                        const file_handle_mapping_vector& fds_to_remap,
-                        bool wait, ProcessHandle* process_handle);
+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
+inline bool LaunchApp(const std::vector<std::string>& argv,
+                      const file_handle_mapping_vector& fds_to_remap,
+                      bool wait, ProcessHandle* process_handle) {
+  LaunchOptions options;
+  options.fds_to_remap = &fds_to_remap;
+  options.wait = wait;
+  options.process_handle = process_handle;
+  return LaunchProcess(argv, options);
+}
 
-// Similar to the above two methods, but starts the child process in a process
-// group of its own, instead of allowing it to inherit the parent's process
-// group. The pgid of the child process will be the same as its pid.
-BASE_API bool LaunchAppInNewProcessGroup(
+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
+inline bool LaunchApp(const std::vector<std::string>& argv,
+                      const environment_vector& environ,
+                      const file_handle_mapping_vector& fds_to_remap,
+                      bool wait, ProcessHandle* process_handle) {
+  LaunchOptions options;
+  options.environ = &environ;
+  options.fds_to_remap = &fds_to_remap;
+  options.wait = wait;
+  options.process_handle = process_handle;
+  return LaunchProcess(argv, options);
+}
+
+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
+inline bool LaunchAppInNewProcessGroup(
     const std::vector<std::string>& argv,
     const environment_vector& environ,
     const file_handle_mapping_vector& fds_to_remap,
     bool wait,
-    ProcessHandle* process_handle);
+    ProcessHandle* process_handle) {
+  LaunchOptions options;
+  options.environ = &environ;
+  options.fds_to_remap = &fds_to_remap;
+  options.wait = wait;
+  options.process_handle = process_handle;
+  options.new_process_group = true;
+  return LaunchProcess(argv, options);
+}
 
 // AlterEnvironment returns a modified environment vector, constructed from the
 // given environment and the list of changes given in |changes|. Each key in
 // the environment is matched against the first element of the pairs. In the
 // event of a match, the value is replaced by the second of the pair, unless
 // the second is empty, in which case the key-value is removed.
 //
 // The returned array is allocated using new[] and must be freed by the caller.
 BASE_API char** AlterEnvironment(const environment_vector& changes,
                                  const char* const* const env);
 #endif  // defined(OS_POSIX)
 
 // Executes the application specified by cl. This function delegates to one
-// of the above two platform-specific functions.
+// of the above platform-specific functions.
+// TODO(evan): deprecated; change callers to use LaunchProcess, remove.
 BASE_API bool LaunchApp(const CommandLine& cl, bool wait, bool start_hidden,
-                        ProcessHandle* process_handle);
+                        ProcessHandle* process_handle) {
+  LaunchOptions options;
+  options.wait = wait;
+  options.process_handle = process_handle;
+
+  return LaunchProcess(cl, options);
+}
 
 // Executes the application specified by |cl| and wait for it to exit. Stores
 // the output (stdout) in |output|. Redirects stderr to /dev/null. Returns true
 // on success (application launched and exited cleanly, with exit code
 // indicating success).
 BASE_API bool GetAppOutput(const CommandLine& cl, std::string* output);
 
 #if defined(OS_POSIX)
 // A restricted version of |GetAppOutput()| which (a) clears the environment,
 // and (b) stores at most |max_output| bytes; also, it doesn't search the path
@@ skipping 361 matching lines @@
 // instance running inside the parent.  The parent's Breakpad instance should
 // not handle the child's exceptions.  Calling RestoreDefaultExceptionHandler
 // in the child after forking will restore the standard exception handler.
 // See http://crbug.com/20371/ for more details.
 void RestoreDefaultExceptionHandler();
 #endif  // defined(OS_MACOSX)
 
 }  // namespace base
 
 #endif  // BASE_PROCESS_UTIL_H_