|
|
DescriptionAdd real_path option to LaunchProcess
When specified, real_path will be used as the location of the executable
to launch instead of argv[0]. This is useful when a custom argv[0] must
be specified.
This is needed by Chrome Remote desktop to start a login shell, which
requires invoking the shell with an argv[0] starting with a '-'
character (e.g., "-bash").
Committed: https://crrev.com/732f03d294cead0635f02d24a3fa656416f58c25
Cr-Commit-Position: refs/heads/master@{#422457}
Patch Set 1 : Add real_path option to LaunchProcess #
Total comments: 1
Patch Set 2 : Make real_path posix only #
Messages
Total messages: 32 (19 generated)
The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am making a change to Chrome Remote Desktop that involves invoking a user log-in shell. The standard way to do this is to invoke the shell with an argv[0] starting with a '-' character (e.g., "-bash"). This is not currently possible to do with LaunchProcess, as LaunchProcess unconditionally uses argv[0] to locate the target executable. This patch adds a real_path field to LaunchOptions, which, when specified, is used to locate the target executable instead of argv[0], allowing argv[0] to be modified as needed. The changes to launch_win.cc could use a review by someone more familiar with Windows process creation, though I believe they do the correct thing based on the documentation. If this functionality is not desired in LaunchProcess, I can alternatively manually handle setting up the environment and calling exec in our code. Thanks.
What's the advantage of using LaunchProcess in your code?
On 2016/09/15 19:03:46, Nico wrote: > What's the advantage of using LaunchProcess in your code? LaunchProcess handles clearing the environment and setting the desired environment variables, performing the fork and exec dance, and handling any errors that might occur along the way, all of which would otherwise have to be duplicated in our code. Additionally, LaunchProcess also ensures that extraneous file descriptors are closed and signal handlers are reset.
On 2016/09/15 20:21:16, rkjnsn wrote: > On 2016/09/15 19:03:46, Nico wrote: > > What's the advantage of using LaunchProcess in your code? > > LaunchProcess handles clearing the environment and setting the desired > environment variables, performing the fork and exec dance, and handling any > errors that might occur along the way, all of which would otherwise have to be > duplicated in our code. Additionally, LaunchProcess also ensures that extraneous > file descriptors are closed and signal handlers are reset. That sounds like a good reason. The CL description is a bit sparse. You say this is to launch a login shell elsewhere, which requires passing "-bash"; move that from the initial email to the commit message. Why can't you pass "-bash" as argv[0]? I guess something else doesn't expect that? (Sorry, I don't have this code swapped in and I haven't found time to swap it in this week.)
Description was changed from ========== Add real_path option to LaunchProcess When specified, real_path will be used as the location of the executable instead of argv[0]. This is useful when a custom argv[0] must be specified. ========== to ========== Add real_path option to LaunchProcess When specified, real_path will be used as the location of the executable to launch instead of argv[0]. This is useful when a custom argv[0] must be specified. This is needed by Chrome Remote desktop to start a login shell, which requires invoking the shell with an argv[0] starting with a '-' character (e.g., "-bash"). ==========
On 2016/09/20 22:02:46, Nico wrote: > On 2016/09/15 20:21:16, rkjnsn wrote: > > On 2016/09/15 19:03:46, Nico wrote: > > > What's the advantage of using LaunchProcess in your code? > > > > LaunchProcess handles clearing the environment and setting the desired > > environment variables, performing the fork and exec dance, and handling any > > errors that might occur along the way, all of which would otherwise have to be > > duplicated in our code. Additionally, LaunchProcess also ensures that > extraneous > > file descriptors are closed and signal handlers are reset. > > That sounds like a good reason. The CL description is a bit sparse. You say this > is to launch a login shell elsewhere, which requires passing "-bash"; move that > from the initial email to the commit message. > > Why can't you pass "-bash" as argv[0]? I guess something else doesn't expect > that? > > (Sorry, I don't have this code swapped in and I haven't found time to swap it in > this week.) Added the motivation for this change to the commit message. The problem with passing "-bash" as argv[0] today is that LaunchProcess will then try to execute a binary called "-bash", which doesn't exist. This patch provides an alternative way to specify the binary to execute, allowing "-bash" to be passed as argv[0] and the actual path to the shell to be provided through "real_path".
Thanks. One more comment below. https://codereview.chromium.org/2338133005/diff/20001/base/process/launch.h File base/process/launch.h (right): https://codereview.chromium.org/2338133005/diff/20001/base/process/launch.h#n... base/process/launch.h:71: // If not empty, launch the specified executable instead of Does this ever make sense on non-posix? Do you know of use cases other than login shells? If not, I'd make this #if defined(OS_POSIX).
On 2016/09/22 17:03:04, Nico wrote: > Thanks. One more comment below. > > https://codereview.chromium.org/2338133005/diff/20001/base/process/launch.h > File base/process/launch.h (right): > > https://codereview.chromium.org/2338133005/diff/20001/base/process/launch.h#n... > base/process/launch.h:71: // If not empty, launch the specified executable > instead of > Does this ever make sense on non-posix? Do you know of use cases other than > login shells? If not, I'd make this #if defined(OS_POSIX). I can think of cases where it might make sense to do this on Windows, thought admittedly none of them seem terribly likely to be encountered by Chrome. If you prefer, I'm happy to make this change POSIX only unless/until it is needed on Windows.
Yes, let's do that then. On Thu, Sep 22, 2016 at 4:27 PM, <rkjnsn@chromium.org> wrote: > On 2016/09/22 17:03:04, Nico wrote: > > Thanks. One more comment below. > > > > https://codereview.chromium.org/2338133005/diff/20001/ > base/process/launch.h > > File base/process/launch.h (right): > > > > > https://codereview.chromium.org/2338133005/diff/20001/ > base/process/launch.h#newcode71 > > base/process/launch.h:71: // If not empty, launch the specified > executable > > instead of > > Does this ever make sense on non-posix? Do you know of use cases other > than > > login shells? If not, I'd make this #if defined(OS_POSIX). > > I can think of cases where it might make sense to do this on Windows, > thought > admittedly none of them seem terribly likely to be encountered by Chrome. > If you > prefer, I'm happy to make this change POSIX only unless/until it is needed > on > Windows. > > https://codereview.chromium.org/2338133005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rkjnsn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add real_path option to LaunchProcess When specified, real_path will be used as the location of the executable to launch instead of argv[0]. This is useful when a custom argv[0] must be specified. This is needed by Chrome Remote desktop to start a login shell, which requires invoking the shell with an argv[0] starting with a '-' character (e.g., "-bash"). ========== to ========== Add real_path option to LaunchProcess When specified, real_path will be used as the location of the executable to launch instead of argv[0]. This is useful when a custom argv[0] must be specified. This is needed by Chrome Remote desktop to start a login shell, which requires invoking the shell with an argv[0] starting with a '-' character (e.g., "-bash"). ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add real_path option to LaunchProcess When specified, real_path will be used as the location of the executable to launch instead of argv[0]. This is useful when a custom argv[0] must be specified. This is needed by Chrome Remote desktop to start a login shell, which requires invoking the shell with an argv[0] starting with a '-' character (e.g., "-bash"). ========== to ========== Add real_path option to LaunchProcess When specified, real_path will be used as the location of the executable to launch instead of argv[0]. This is useful when a custom argv[0] must be specified. This is needed by Chrome Remote desktop to start a login shell, which requires invoking the shell with an argv[0] starting with a '-' character (e.g., "-bash"). Committed: https://crrev.com/732f03d294cead0635f02d24a3fa656416f58c25 Cr-Commit-Position: refs/heads/master@{#422457} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/732f03d294cead0635f02d24a3fa656416f58c25 Cr-Commit-Position: refs/heads/master@{#422457} |