Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161013003:
Changing the default path for RemoteSecurityKeyIpcServer test channels (Closed)
DescriptionChanging the default path for RemoteSecurityKeyIpcServer test channels
The following change moved the location of our IPC test channels to the
home directory which is why we were seeing left-over crud there when the
tests were cancelled:
Change 4e752e108394090a1d83f9054a862ffa60293346
https://codereview.chromium.org/2003753002
My change moves the location from the home directory, which can cause
problems for local development to the temp directory which should work
for both local devs and trybots. I've also included other posix based
OSes in this change since their IPC channels use UDS as well (Windows
should not be changed since it still uses named pipes).
BUG=621995
Committed: https://crrev.com/efeac5ea44139d44a279a2ad6b5dae9c66543bc4
Cr-Commit-Position: refs/heads/master@{#406759}
Patch Set 1 #Patch Set 2 : Removing a file that was accidentally included. #
Total comments: 9
Patch Set 3 : Addressing feedback #
Total comments: 2
Patch Set 4 : Fixing some header includes and comments #Patch Set 5 : Merging with ToT #Patch Set 6 : Fixing a potential problem where the socket name is longer than 104 chars #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 34 (22 generated)
The CQ bit was checked by joedow@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...
joedow@chromium.org changed reviewers: + sergeyu@chromium.org, thestig@chromium.org
PTAL! sergeyu@ for the remoting change, thestig@ for base change. Thanks! Joe
https://codereview.chromium.org/2161013003/diff/20001/base/environment.h File base/environment.h (right): https://codereview.chromium.org/2161013003/diff/20001/base/environment.h#newc... base/environment.h:23: BASE_EXPORT extern const char kTempDir[]; Does this need to be in base instead of somewhere in src/remoting?
Changes to base/environment.* make my spider senses tangle. https://codereview.chromium.org/2161013003/diff/20001/remoting/host/security_... File remoting/host/security_key/security_key_ipc_constants.cc (right): https://codereview.chromium.org/2161013003/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_constants.cc:20: extern const char kSecurityKeyConnectionError[] = "ssh_connection_error"; Drive by: "extern" should not be used in .cc files. https://codereview.chromium.org/2161013003/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_constants.cc:39: if (env->GetVar(base::env_vars::kTempDir, &path_prefix)) Can you just call base::GetTempDir() instead? If your tests has trouble cleaning up after itself for whatever reason, have you tried using a base::ScopedTempDir, and putting everything in there? That way, when the ScopedTempDir gets destroyed, everything inside goes away. (Not 100% sure about Windows if there are still open files in the ScopedTempDir.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2161013003/diff/20001/remoting/host/security_... File remoting/host/security_key/security_key_ipc_constants.cc (right): https://codereview.chromium.org/2161013003/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_constants.cc:39: if (env->GetVar(base::env_vars::kTempDir, &path_prefix)) Looks like we have base::GetTempDir() in base/files/file_util.h . Use it here instead accessing env directly?
The CQ bit was checked by joedow@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...
Since base/ is no longer modified, I'll just leave a comment and be on my way. https://codereview.chromium.org/2161013003/diff/40001/remoting/host/security_... File remoting/host/security_key/security_key_ipc_constants.cc (right): https://codereview.chromium.org/2161013003/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_ipc_constants.cc:10: #include "base/macros.h" It's great that you are trying to IWYU, but what is this for? OS_POSIX? LOG(ERROR) ? Either way, this is the wrong header. OS_POSIX is in build/build_config.h LOG(ERROR) is in base/logging.h
The CQ bit was checked by joedow@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...
Addressed feedback, PTAL! https://codereview.chromium.org/2161013003/diff/20001/base/environment.h File base/environment.h (right): https://codereview.chromium.org/2161013003/diff/20001/base/environment.h#newc... base/environment.h:23: BASE_EXPORT extern const char kTempDir[]; On 2016/07/19 22:00:51, Sergey Ulanov wrote: > Does this need to be in base instead of somewhere in src/remoting? My change here was under the assumption that adding well-known env var names would be useful for others. Since I am going a different route I just removed this bit here. https://codereview.chromium.org/2161013003/diff/20001/remoting/host/security_... File remoting/host/security_key/security_key_ipc_constants.cc (right): https://codereview.chromium.org/2161013003/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_constants.cc:20: extern const char kSecurityKeyConnectionError[] = "ssh_connection_error"; On 2016/07/19 22:07:30, Lei Zhang wrote: > Drive by: "extern" should not be used in .cc files. Done. https://codereview.chromium.org/2161013003/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_constants.cc:39: if (env->GetVar(base::env_vars::kTempDir, &path_prefix)) On 2016/07/19 22:34:57, Sergey Ulanov wrote: > Looks like we have base::GetTempDir() in base/files/file_util.h . Use it here > instead accessing env directly? Done. https://codereview.chromium.org/2161013003/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_ipc_constants.cc:39: if (env->GetVar(base::env_vars::kTempDir, &path_prefix)) On 2016/07/19 22:07:30, Lei Zhang wrote: > Can you just call base::GetTempDir() instead? > > If your tests has trouble cleaning up after itself for whatever reason, have you > tried using a base::ScopedTempDir, and putting everything in there? That way, > when the ScopedTempDir gets destroyed, everything inside goes away. (Not 100% > sure about Windows if there are still open files in the ScopedTempDir.) Done. https://codereview.chromium.org/2161013003/diff/40001/remoting/host/security_... File remoting/host/security_key/security_key_ipc_constants.cc (right): https://codereview.chromium.org/2161013003/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_ipc_constants.cc:10: #include "base/macros.h" On 2016/07/19 22:55:04, Lei Zhang wrote: > It's great that you are trying to IWYU, but what is this for? OS_POSIX? > LOG(ERROR) ? Either way, this is the wrong header. > > OS_POSIX is in build/build_config.h > LOG(ERROR) is in base/logging.h Done.
https://codereview.chromium.org/2161013003/diff/20001/base/environment.h File base/environment.h (right): https://codereview.chromium.org/2161013003/diff/20001/base/environment.h#newc... base/environment.h:23: BASE_EXPORT extern const char kTempDir[]; On 2016/07/19 23:04:12, joedow wrote: > On 2016/07/19 22:00:51, Sergey Ulanov wrote: > > Does this need to be in base instead of somewhere in src/remoting? > > My change here was under the assumption that adding well-known env var names > would be useful for others. Since I am going a different route I just removed > this bit here. It turns out we steer everyone to use GetTempDir(). So it is only ever used in 1 place. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by joedow@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by joedow@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.
Thanks for all the feedback!
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2161013003/#ps100001 (title: "Fixing a potential problem where the socket name is longer than 104 chars")
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.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Changing the default path for RemoteSecurityKeyIpcServer test channels The following change moved the location of our IPC test channels to the home directory which is why we were seeing left-over crud there when the tests were cancelled: Change 4e752e108394090a1d83f9054a862ffa60293346 https://codereview.chromium.org/2003753002 My change moves the location from the home directory, which can cause problems for local development to the temp directory which should work for both local devs and trybots. I've also included other posix based OSes in this change since their IPC channels use UDS as well (Windows should not be changed since it still uses named pipes). BUG=621995 ========== to ========== Changing the default path for RemoteSecurityKeyIpcServer test channels The following change moved the location of our IPC test channels to the home directory which is why we were seeing left-over crud there when the tests were cancelled: Change 4e752e108394090a1d83f9054a862ffa60293346 https://codereview.chromium.org/2003753002 My change moves the location from the home directory, which can cause problems for local development to the temp directory which should work for both local devs and trybots. I've also included other posix based OSes in this change since their IPC channels use UDS as well (Windows should not be changed since it still uses named pipes). BUG=621995 Committed: https://crrev.com/efeac5ea44139d44a279a2ad6b5dae9c66543bc4 Cr-Commit-Position: refs/heads/master@{#406759} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/efeac5ea44139d44a279a2ad6b5dae9c66543bc4 Cr-Commit-Position: refs/heads/master@{#406759} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
