Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778173003:
base: Support command line wrappers with quoted arguments (Closed)
Descriptionbase: Support command line wrappers with quoted arguments
Command line wrappers (e.g., --renderer-cmd-prefix) were previously
split into individual arguments purely based on whitespace. This fails
if the prefix has arguments with embedded whitespace (e.g., gdb -ex "set
height 0").
This patch fixes the issue by tokenizing the prefix based on whitespace
while using ' and " as quote characters.
BUG=546953
Review-Url: https://codereview.chromium.org/2778173003
Cr-Commit-Position: refs/heads/master@{#460443}
Committed: https://chromium.googlesource.com/chromium/src/+/d851aa1e5290a777a7a82afdbe6c27df2cf82bf2
Patch Set 1 #
Total comments: 6
Patch Set 2 : Windows fixes #Patch Set 3 : More windows fixes #Patch Set 4 : Yet more windows fixes #
Total comments: 6
Patch Set 5 : Review comments #Messages
Total messages: 31 (22 generated)
Description was changed from ========== base: Support command line wrappers with quoted arguments Command line wrappers (e.g., --renderer-cmd-prefix) were previously split into individual arguments purely based on whitespace. This fails if the prefix has arguments with embedded whitespace (e.g., gdb -ex "set height 0"). This patch fixes the issue by tokenizing the prefix based on whitespace while using ' and " as quote characters. BUG=546953 ========== to ========== base: Support command line wrappers with quoted arguments Command line wrappers (e.g., --renderer-cmd-prefix) were previously split into individual arguments purely based on whitespace. This fails if the prefix has arguments with embedded whitespace (e.g., gdb -ex "set height 0"). This patch fixes the issue by tokenizing the prefix based on whitespace while using ' and " as quote characters. BUG=546953 ==========
skyostil@chromium.org changed reviewers: + gab@chromium.org
The CQ bit was checked by skyostil@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...
COol thanks https://codereview.chromium.org/2778173003/diff/1/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2778173003/diff/1/base/command_line.cc#newcod... base/command_line.cc:416: StringTokenizer tokenizer(wrapper, " "); This won't compile on Windows I think (CommandLine::StringType will be string16) using CommandLineTokenizer = StringTokenizerT<CommandLine::StringType, CommandLine::StringType::const_iterator>; will probably work? https://codereview.chromium.org/2778173003/diff/1/base/command_line.cc#newcod... base/command_line.cc:421: it = argv_.emplace(it, tokenizer.token()) + 1; Pushing in front of vector repeatedly is inefficient, build outside of loop and insert in one pass like SplitString() approach effectively used to do? https://codereview.chromium.org/2778173003/diff/1/base/command_line.cc#newcod... base/command_line.cc:422: begin_args_++; Irrelevant after above comment but FWIW: prefer pre-increment when it's equivalent
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by skyostil@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 checked by skyostil@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...
Thanks! https://codereview.chromium.org/2778173003/diff/1/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2778173003/diff/1/base/command_line.cc#newcod... base/command_line.cc:416: StringTokenizer tokenizer(wrapper, " "); On 2017/03/28 17:03:59, gab wrote: > This won't compile on Windows I think (CommandLine::StringType will be string16) > > using CommandLineTokenizer = StringTokenizerT<CommandLine::StringType, > CommandLine::StringType::const_iterator>; > > will probably work? Ah, thanks for the tip! Done. https://codereview.chromium.org/2778173003/diff/1/base/command_line.cc#newcod... base/command_line.cc:421: it = argv_.emplace(it, tokenizer.token()) + 1; On 2017/03/28 17:03:59, gab wrote: > Pushing in front of vector repeatedly is inefficient, build outside of loop and > insert in one pass like SplitString() approach effectively used to do? Right you are -- done. https://codereview.chromium.org/2778173003/diff/1/base/command_line.cc#newcod... base/command_line.cc:422: begin_args_++; On 2017/03/28 17:04:00, gab wrote: > Irrelevant after above comment but FWIW: prefer pre-increment when it's > equivalent Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by skyostil@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 w/ nits https://codereview.chromium.org/2778173003/diff/60001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2778173003/diff/60001/base/command_line.cc#ne... base/command_line.cc:417: StringTokenizerT<StringType, CommandLine::StringType::const_iterator>; can we forgo the CommandLine:: namespace prefix in const_iterator as well? https://codereview.chromium.org/2778173003/diff/60001/base/command_line.cc#ne... base/command_line.cc:418: CommandLineTokenizer tokenizer(wrapper, FILE_PATH_LITERAL(" ")); Unfortunate to use FILE_PATH_LITERAL on a non-file-path argument but looking at the rest of this file I guess we're committed to that..! https://codereview.chromium.org/2778173003/diff/60001/base/command_line.cc#ne... base/command_line.cc:424: // Prepend the wrapper and update the switches/arguments |begin_args_| nit: keep '.' at end of comment
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2778173003/diff/60001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/2778173003/diff/60001/base/command_line.cc#ne... base/command_line.cc:417: StringTokenizerT<StringType, CommandLine::StringType::const_iterator>; On 2017/03/28 19:31:46, gab wrote: > can we forgo the CommandLine:: namespace prefix in const_iterator as well? Well spotted, done. https://codereview.chromium.org/2778173003/diff/60001/base/command_line.cc#ne... base/command_line.cc:418: CommandLineTokenizer tokenizer(wrapper, FILE_PATH_LITERAL(" ")); On 2017/03/28 19:31:46, gab wrote: > Unfortunate to use FILE_PATH_LITERAL on a non-file-path argument but looking at > the rest of this file I guess we're committed to that..! Yep, doesn't look like there's any neat alternative :P https://codereview.chromium.org/2778173003/diff/60001/base/command_line.cc#ne... base/command_line.cc:424: // Prepend the wrapper and update the switches/arguments |begin_args_| On 2017/03/28 19:31:46, gab wrote: > nit: keep '.' at end of comment Done.
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2778173003/#ps80001 (title: "Review comments")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by skyostil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490804212050930,
"parent_rev": "158d97ab48e2c8343051e439db4f470151039ee5", "commit_rev":
"d851aa1e5290a777a7a82afdbe6c27df2cf82bf2"}
Message was sent while issue was closed.
Description was changed from ========== base: Support command line wrappers with quoted arguments Command line wrappers (e.g., --renderer-cmd-prefix) were previously split into individual arguments purely based on whitespace. This fails if the prefix has arguments with embedded whitespace (e.g., gdb -ex "set height 0"). This patch fixes the issue by tokenizing the prefix based on whitespace while using ' and " as quote characters. BUG=546953 ========== to ========== base: Support command line wrappers with quoted arguments Command line wrappers (e.g., --renderer-cmd-prefix) were previously split into individual arguments purely based on whitespace. This fails if the prefix has arguments with embedded whitespace (e.g., gdb -ex "set height 0"). This patch fixes the issue by tokenizing the prefix based on whitespace while using ' and " as quote characters. BUG=546953 Review-Url: https://codereview.chromium.org/2778173003 Cr-Commit-Position: refs/heads/master@{#460443} Committed: https://chromium.googlesource.com/chromium/src/+/d851aa1e5290a777a7a82afdbe6c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d851aa1e5290a777a7a82afdbe6c... |
