Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1186873006:
Add Get/SetUniqueIdForProcess. (Closed)
|
Created:
5 years, 6 months ago by rickyz (no longer on Chrome) Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, erikwright+watch_chromium.org, jam, rickyz+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Get/SetUniqueIdForProcess.
base::GetCurrentProcId is sometimes assumed to return a unique
identifier for a process. With PID namespces, each renderer has PID 1,
which is makes the PID for this purpose. This change introduces a
supported way to obtain a unique ID for a process. Currently, the ID is
the process's PID outside of the PID namespace.
BUG=501069
Committed: https://crrev.com/5b937a380101191cdd460033f4b72394b6638798
Cr-Commit-Position: refs/heads/master@{#355440}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Respond to comments. #
Total comments: 4
Patch Set 3 : Respond to comments. #
Total comments: 8
Patch Set 4 : Respond to comments. #
Total comments: 19
Patch Set 5 : Respond to comments. #
Total comments: 1
Patch Set 6 : Rebase #
Total comments: 4
Patch Set 7 : Use uint32_t, prettify comments #
Total comments: 16
Patch Set 8 : Respond to comments. #Patch Set 9 : Mangle unique IDs so that they cannot accidentally be used as PIDs. #Patch Set 10 : Fix outdated comment #Patch Set 11 : Get rid of DCHECK_IS_ON. #
Total comments: 4
Patch Set 12 : Respond to comments #Patch Set 13 : Rebase #
Messages
Total messages: 60 (9 generated)
rickyz@chromium.org changed reviewers: + jln@chromium.org, mdempsky@chromium.org
https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle.cc File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle... base/process/process_handle.cc:7: #include <unistd.h> I don't see any use of unistd.h? (I also don't think it exists on Windows?) https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle.h File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle... base/process/process_handle.h:43: // Returns a globally unique ID for the current process. This defaults to the I think "globally" might be misinterpretable. It might be better to clarify that the ID is unique across all live processes within the chrome session. (I don't think we're making a stronger guarantee than that? I.e., other chrome sessions can in the future reuse IDs, and if a process exits its ID can be reused.) https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle... base/process/process_handle.h:44: // process process ID, but may return a different value if SetUniqueIdForProcess Should "process process ID" be "process's process ID"?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle.cc File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle... base/process/process_handle.cc:7: #include <unistd.h> On 2015/06/15 21:53:09, mdempsky wrote: > I don't see any use of unistd.h? (I also don't think it exists on Windows?) Oops, removed. https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle.h File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle... base/process/process_handle.h:43: // Returns a globally unique ID for the current process. This defaults to the On 2015/06/15 21:53:09, mdempsky wrote: > I think "globally" might be misinterpretable. It might be better to clarify > that the ID is unique across all live processes within the chrome session. (I > don't think we're making a stronger guarantee than that? I.e., other chrome > sessions can in the future reuse IDs, and if a process exits its ID can be > reused.) Ah, good point, changed. https://codereview.chromium.org/1186873006/diff/1/base/process/process_handle... base/process/process_handle.h:44: // process process ID, but may return a different value if SetUniqueIdForProcess On 2015/06/15 21:53:09, mdempsky wrote: > Should "process process ID" be "process's process ID"? Done.
Seems good in general, but thread-safety is an issue. Probably using a lazy instance would be a good idea: The lazy instance is initialized either at the first Get/SetUniqueIdForProcess(). Calling SetUniqueIdForProcess() once the instance is initialized should crash. This would make sure that the process never get two different answers when calling GetUniqueIdForProcess(). https://codereview.chromium.org/1186873006/diff/60001/base/process/process_ha... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/60001/base/process/process_ha... base/process/process_handle.cc:22: g_unique_id = unique_id; This is not thread_safe. How about using a lazy instance which deals with that for you? https://codereview.chromium.org/1186873006/diff/60001/base/process/process_ha... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/60001/base/process/process_ha... base/process/process_handle.h:54: // after forking as possible. s/forking/process creation/ just to be friendly to non-Linux.
Also: could you link a bug explaining the overall problem and the solution being built?
Also: in DEBUG builds, could you store the current pid alongside the unique id? Then in DEBUG builds you can check if the current pid changed and crash (which should catch someone doing fork() after SetUniqueIdForProcess() was called.
On 2015/06/15 23:33:28, jln wrote: > Also: in DEBUG builds, could you store the current pid alongside the unique id? > Then in DEBUG builds you can check if the current pid changed and crash (which > should catch someone doing fork() after SetUniqueIdForProcess() was called. Ah yeah, mdempsky@ mentioned doing this as well - added.
https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... base/process/process_handle.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. New file, so remove (c) and use 2015 https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... base/process/process_handle.cc:6: #include "base/process/process_handle.h" Don't forget basictypes.h https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... base/process/process_handle.h:60: // Returns the unique ID for the specified process. This is functionally the Maybe change the comment here saying that it's not necessarily unique? https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... base/process/process_handle.h:63: // DEPRECATED. New code should be using Process::Pid() instead. Should we add a comment that Process::Pid() is a valid process id (and can be used with relevant APIs), but is not necessarily unique across Chrome?
https://codereview.chromium.org/1186873006/diff/60001/base/process/process_ha... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/60001/base/process/process_ha... base/process/process_handle.cc:22: g_unique_id = unique_id; On 2015/06/15 23:25:08, jln wrote: > This is not thread_safe. How about using a lazy instance which deals with that > for you? Does it suffice to just document that it is not thread safe? It doesn't make sense to call this function from multiple threads anyway. https://codereview.chromium.org/1186873006/diff/60001/base/process/process_ha... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/60001/base/process/process_ha... base/process/process_handle.h:54: // after forking as possible. On 2015/06/15 23:25:08, jln wrote: > s/forking/process creation/ just to be friendly to non-Linux. Done. https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... base/process/process_handle.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2015/06/16 22:59:01, jln wrote: > New file, so remove (c) and use 2015 Done. https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... base/process/process_handle.cc:6: #include "base/process/process_handle.h" On 2015/06/16 22:59:01, jln wrote: > Don't forget basictypes.h Done. https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... base/process/process_handle.h:60: // Returns the unique ID for the specified process. This is functionally the On 2015/06/16 22:59:01, jln wrote: > Maybe change the comment here saying that it's not necessarily unique? Done, and added a comment to GetCurrentProcId. https://codereview.chromium.org/1186873006/diff/80001/base/process/process_ha... base/process/process_handle.h:63: // DEPRECATED. New code should be using Process::Pid() instead. On 2015/06/16 22:59:01, jln wrote: > Should we add a comment that Process::Pid() is a valid process id (and can be > used with relevant APIs), but is not necessarily unique across Chrome? I didn't mention Process::Pid(), since that sounded a bit tautological, but I mentioned that these aren't guaranteed to be unique on some platforms.
lgtm We might want to use a uint64 instead of a uint32, but other than this, it should be fine.
rickyz@chromium.org changed reviewers: + willchan@chromium.org
Hey, hope you don't mind taking a look at this small change :-) The background is that some parts of chromium rely on process IDs being unique, but with PID namespaces on Linux, this is no longer the case. To deal with this, we introduce a new function for people who want a unique identifier (vs. something people can use as a pid for stuff like waitpid and such)
willchan@chromium.org changed reviewers: + danakj@chromium.org
Sorry, no longer on Chromium. Redirecting to Dana. Hi Dana! https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:25: #ifndef NDEBUG Why bother with NDEBUG if using DCHECK_EQ? Is there another side effect I'm missing? https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:35: g_unique_id = unique_id; This shouldn't be set twice, right? I wonder if it'd be good to DCHECK_EQ(kInvalidUniqueId, g_unique_id);
Sorry, no longer on Chromium. Redirecting to Dana. Hi Dana!
Thanks for the comments! https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:25: #ifndef NDEBUG On 2015/06/17 01:51:44, willchan no longer on Chromium wrote: > Why bother with NDEBUG if using DCHECK_EQ? Is there another side effect I'm > missing? I kept the explicit ifndef because I saw that there was a DCHECK_ALWAYS_ON macro that could enable DCHECKs even with NDEBUG is set - also, if I read correctly, it looks like DCHECK_EQ will still evaluate to code that references the variable - it just relies on the compiler to omit that code since it ends up inside an if (0). https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:35: g_unique_id = unique_id; On 2015/06/17 01:51:45, willchan no longer on Chromium wrote: > This shouldn't be set twice, right? I wonder if it'd be good to > DCHECK_EQ(kInvalidUniqueId, g_unique_id); In practice, this really shouldn't be set twice - I left in the possibility because a child process could potentially fork again and want to set this - in that case though, the child would need to ensure that it doesn't end up relying on any references to the old unique id in memory, which seems a little fragile - jln@, what are your thoughts on this?
https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:25: #ifndef NDEBUG On 2015/06/17 03:04:06, rickyz wrote: > On 2015/06/17 01:51:44, willchan no longer on Chromium wrote: > > Why bother with NDEBUG if using DCHECK_EQ? Is there another side effect I'm > > missing? > I kept the explicit ifndef because I saw that there was a DCHECK_ALWAYS_ON macro > that could enable DCHECKs even with NDEBUG is set - also, if I read correctly, > it looks like DCHECK_EQ will still evaluate to code that references the variable > - it just relies on the compiler to omit that code since it ends up inside an if > (0). It avoids unused variable warnings when compiled out, but does not run the code inside. DCHECK_IS_ON means we want dchecks, otherwise we don't. Why do you only want this dcheck in a debug build? Then it will not run on the bots.
https://codereview.chromium.org/1186873006/diff/100001/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1186873006/diff/100001/base/base.gypi#newcode477 base/base.gypi:477: 'process/process_handle.cc', add the header here too, it was omitted. https://codereview.chromium.org/1186873006/diff/100001/base/process/BUILD.gn File base/process/BUILD.gn (right): https://codereview.chromium.org/1186873006/diff/100001/base/process/BUILD.gn#... base/process/BUILD.gn:26: "process_handle.cc", add the header here too, it was omitted https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:22: return static_cast<uint32>(GetCurrentProcId()); Should SetUniqueId(GetCurrentProcId()) here so we don't return inconsistent things? https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:25: #ifndef NDEBUG On 2015/06/17 17:09:53, danakj wrote: > On 2015/06/17 03:04:06, rickyz wrote: > > On 2015/06/17 01:51:44, willchan no longer on Chromium wrote: > > > Why bother with NDEBUG if using DCHECK_EQ? Is there another side effect I'm > > > missing? > > I kept the explicit ifndef because I saw that there was a DCHECK_ALWAYS_ON > macro > > that could enable DCHECKs even with NDEBUG is set - also, if I read correctly, > > it looks like DCHECK_EQ will still evaluate to code that references the > variable > > - it just relies on the compiler to omit that code since it ends up inside an > if > > (0). > > It avoids unused variable warnings when compiled out, but does not run the code > inside. DCHECK_IS_ON means we want dchecks, otherwise we don't. Why do you only > want this dcheck in a debug build? Then it will not run on the bots. You can guard all your NDEBUG code with #if DCHECK_IS_ON() instead and it'll do the right thing. https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:37: g_procid = static_cast<uint32>(GetCurrentProcId()); How useful is this if GetCurrentProcId() is always 1? https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.h:37: // This is not a valid process ID on Linux, Mac, or Windows. Is it on android? https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.h:54: // WARNING: To avoid inconsistent results from GetUniqueIdForProcess, this Can you DCHECK that when SetUniqueId is called, GetUniqueId hasn't been called yet? https://codereview.chromium.org/1186873006/diff/100001/content/zygote/zygote_... File content/zygote/zygote_linux.cc (right): https://codereview.chromium.org/1186873006/diff/100001/content/zygote/zygote_... content/zygote/zygote_linux.cc:443: base::SetUniqueIdForProcess(static_cast<uint32>(real_pid)); What about other platforms?
https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:22: return static_cast<uint32>(GetCurrentProcId()); On 2015/06/17 17:17:16, danakj wrote: > Should SetUniqueId(GetCurrentProcId()) here so we don't return inconsistent > things? If we call SetUniqueIdForProcess here, then GetUniqueIdForProcess would return an incorrect value after the process forks (which is also the only situation where an inconsistent value might be returned by GetCurrentProcId). It'd also make GetCurrentProcId non thread-safe. I'm kind of on the fence about whether we should support forking after calling SetUniqueIdForProcess at all (see the response to willchan@'s comment in SetUniqueIdForProcess), but leaning towards leaving things more flexible for the moment. https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:25: #ifndef NDEBUG On 2015/06/17 17:17:16, danakj wrote: > On 2015/06/17 17:09:53, danakj wrote: > > On 2015/06/17 03:04:06, rickyz wrote: > > > On 2015/06/17 01:51:44, willchan no longer on Chromium wrote: > > > > Why bother with NDEBUG if using DCHECK_EQ? Is there another side effect > I'm > > > > missing? > > > I kept the explicit ifndef because I saw that there was a DCHECK_ALWAYS_ON > > macro > > > that could enable DCHECKs even with NDEBUG is set - also, if I read > correctly, > > > it looks like DCHECK_EQ will still evaluate to code that references the > > variable > > > - it just relies on the compiler to omit that code since it ends up inside > an > > if > > > (0). > > > > It avoids unused variable warnings when compiled out, but does not run the > code > > inside. DCHECK_IS_ON means we want dchecks, otherwise we don't. Why do you > only > > want this dcheck in a debug build? Then it will not run on the bots. > > You can guard all your NDEBUG code with #if DCHECK_IS_ON() instead and it'll do > the right thing. Ah, good point. https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.cc:37: g_procid = static_cast<uint32>(GetCurrentProcId()); On 2015/06/17 17:17:16, danakj wrote: > How useful is this if GetCurrentProcId() is always 1? GetCurrentProcId is only 1 in processes which are in separate PID namespaces, so for example, it'd return 1 on renderer processes if PID namespaces are supported, but some other number on the browser process. https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.h:37: // This is not a valid process ID on Linux, Mac, or Windows. On 2015/06/17 17:17:16, danakj wrote: > Is it on android? Yeah, I considered that to be part of Linux since this is a kernel detail - added a mention to the comment. https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.h:54: // WARNING: To avoid inconsistent results from GetUniqueIdForProcess, this On 2015/06/17 17:17:16, danakj wrote: > Can you DCHECK that when SetUniqueId is called, GetUniqueId hasn't been called > yet? It's not required to call SetUniqueId before GetUniqueId because it defaults to the process id - we only call SetUniqueId if separate PID namespaces are being used on Linux, which is what causes the getpid() = 1 situation. Would folks prefer not to have a default behavior and to always require that SetUniqueId be called even if it's to set it to the pid? I avoided doing that for this change because it'd involve adding calls to every place (on all platforms) where we fork.
https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... base/process/process_handle.h:54: // WARNING: To avoid inconsistent results from GetUniqueIdForProcess, this On 2015/06/17 19:53:41, rickyz wrote: > On 2015/06/17 17:17:16, danakj wrote: > > Can you DCHECK that when SetUniqueId is called, GetUniqueId hasn't been called > > yet? > It's not required to call SetUniqueId before GetUniqueId because it defaults to > the process id - we only call SetUniqueId if separate PID namespaces are being > used on Linux, which is what causes the getpid() = 1 situation. Would folks > prefer not to have a default behavior and to always require that SetUniqueId be > called even if it's to set it to the pid? I avoided doing that for this change > because it'd involve adding calls to every place (on all platforms) where we > fork. What I'd like is for GetUniqueId to always return a single thing for every call, and situations that would deviate would trigger a DCHECK. Ever getting 2 different return values sounds like a bug.
Ah, though the thing is that after forking, it needs to return a different call value - otherwise it wouldn't be unique between processes. On Wed, Jun 17, 2015, 12:56 <danakj@chromium.org> wrote: > > > https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... > File base/process/process_handle.h (right): > > > https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h... > base/process/process_handle.h:54 > <https://codereview.chromium.org/1186873006/diff/100001/base/process/process_h...>: > // WARNING: To avoid inconsistent > results from GetUniqueIdForProcess, this > On 2015/06/17 19:53:41, rickyz wrote: > > On 2015/06/17 17:17:16, danakj wrote: > > > Can you DCHECK that when SetUniqueId is called, GetUniqueId hasn't > been called > > > yet? > > It's not required to call SetUniqueId before GetUniqueId because it > defaults to > > the process id - we only call SetUniqueId if separate PID namespaces > are being > > used on Linux, which is what causes the getpid() = 1 situation. Would > folks > > prefer not to have a default behavior and to always require that > SetUniqueId be > > called even if it's to set it to the pid? I avoided doing that for > this change > > because it'd involve adding calls to every place (on all platforms) > where we > > fork. > > What I'd like is for GetUniqueId to always return a single thing for > every call, and situations that would deviate would trigger a DCHECK. > Ever getting 2 different return values sounds like a bug. > > https://codereview.chromium.org/1186873006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ah right, forking. OK. On 2015/06/17 01:04:36, rickyz wrote: > Hey, hope you don't mind taking a look at this small change :-) > > The background is that some parts of chromium rely on process IDs being unique, > but with PID namespaces on Linux, this is no longer the case. To deal with this, > we introduce a new function for people who want a unique identifier (vs. > something people can use as a pid for stuff like waitpid and such) Here's an example CL that dealt with this problem. It replaced the use of GetCurrentProcId with something else. Should we do something like this in places that want to use GetCurrentProcId? It feels a bit weird that we're adding some random unique id to base/process_handle that has nothing to do with the process id actually, and is only here because people were previously using a process id for their unique id?
On 2015/06/18 17:16:07, danakj wrote: > Ah right, forking. OK. > > On 2015/06/17 01:04:36, rickyz wrote: > > Hey, hope you don't mind taking a look at this small change :-) > > > > The background is that some parts of chromium rely on process IDs being > unique, > > but with PID namespaces on Linux, this is no longer the case. To deal with > this, > > we introduce a new function for people who want a unique identifier (vs. > > something people can use as a pid for stuff like waitpid and such) > > Here's an example CL that dealt with this problem. It replaced the use of > GetCurrentProcId with something else. Should we do something like this in places > that want to use GetCurrentProcId? It feels a bit weird that we're adding some > random unique id to base/process_handle that has nothing to do with the process > id actually, and is only here because people were previously using a process id > for their unique id? Oops, here's the example: https://codereview.chromium.org/942933005/diff/20001/ipc/mojo/ipc_channel_moj...
On 2015/06/18 17:16:19, danakj wrote: > > Here's an example CL that dealt with this problem. It replaced the use of > > GetCurrentProcId with something else. Should we do something like this in > places > > that want to use GetCurrentProcId? It feels a bit weird that we're adding some > > random unique id to base/process_handle that has nothing to do with the > process > > id actually, and is only here because people were previously using a process > id > > for their unique id? > > Oops, here's the example: > https://codereview.chromium.org/942933005/diff/20001/ipc/mojo/ipc_channel_moj... I'd like to do the equivalent in things that need a unique identifier, but not all places that try to get one have easy access to an IPC channel, which is the main place that has access to a unique identifier for the process. For example, the main place where this is causing an issue is https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... The other purpose of putting the function in process_handle.h is to make people aware that PIDs may not be unique and to give them an alternative - this wouldn't have helped in the webkit case, but hopefully it can help prevent further bugs like this being introduced.
rickyz@chromium.org changed reviewers: + thestig@chromium.org
Friendly ping - also adding thestig@ who noticed that log messages were always showing a pid of 1 now. We could probably use this in logging.cc to get a more useful identifier for log messages.
Ah, ;ooks like you noticed the "pid 1" issue a long time ago. https://chromiumcodereview.appspot.com/1186873006/diff/140001/base/process/pr... File base/process/process_handle.cc (right): https://chromiumcodereview.appspot.com/1186873006/diff/140001/base/process/pr... base/process/process_handle.cc:15: // The process which set g_unique_id. Please refer to variables are |foo|. Ditto below. https://chromiumcodereview.appspot.com/1186873006/diff/140001/content/zygote/... File content/zygote/zygote_linux.cc (right): https://chromiumcodereview.appspot.com/1186873006/diff/140001/content/zygote/... content/zygote/zygote_linux.cc:433: base::SetUniqueIdForProcess(static_cast<uint32>(real_pid)); base/basictypes.h says to use uint32_t instead, ditto elsewhere.
*looks*
https://codereview.chromium.org/1186873006/diff/140001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/140001/base/process/process_h... base/process/process_handle.cc:15: // The process which set g_unique_id. On 2015/07/28 21:54:23, Lei Zhang wrote: > Please refer to variables are |foo|. Ditto below. Done. https://codereview.chromium.org/1186873006/diff/140001/content/zygote/zygote_... File content/zygote/zygote_linux.cc (right): https://codereview.chromium.org/1186873006/diff/140001/content/zygote/zygote_... content/zygote/zygote_linux.cc:433: base::SetUniqueIdForProcess(static_cast<uint32>(real_pid)); On 2015/07/28 21:54:23, Lei Zhang wrote: > base/basictypes.h says to use uint32_t instead, ditto elsewhere. Done.
Friendly ping?
On 2015/08/05 16:02:51, jln wrote: > Friendly ping? Pong? Do you want me to review? I was added as a FYI.
https://codereview.chromium.org/1186873006/diff/120001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/120001/base/process/process_h... base/process/process_handle.cc:28: DCHECK_EQ(static_cast<uint32>(GetCurrentProcId()), g_procid); you'll have to wrap this in DCHECK_IS_ON() too, or else when it's not on, the variable will not exist and it won't compile. https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.cc:23: return static_cast<uint32_t>(GetCurrentProcId()); Is this branch basically for the browser process/other OS? Can we just call SetUniqueIdForProcess always, and this always returns the set id? https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.cc:34: g_unique_id = unique_id; can you DCHECK_GT(unique_id, 0) ? https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.h:8: #include <stdint.h> You want base/basictypes.h, instead of these. https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.h:39: const uint32_t kInvalidUniqueId = 0; Does this need to be in the header? Can it move to the .cc file? Does other code need to use this? https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.h:42: // Note that on some platforms, this is not guaranteed to be unique across Who would ever want to use this one? https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.h:67: // Note that on some platforms, this is not guaranteed to be unique across Does this mean Process:Pid() can be non-unique (always 1) as well? Can we fix it? https://codereview.chromium.org/1186873006/diff/160001/content/zygote/zygote_... File content/zygote/zygote_linux.cc (right): https://codereview.chromium.org/1186873006/diff/160001/content/zygote/zygote_... content/zygote/zygote_linux.cc:426: #if defined(OS_LINUX) btw.. this file is zygote_linux.cc why does it have #if defined(OS_LINUX) as well?
Thanks for the comments - sorry about the slow response - just got back from some traveling. https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.cc:23: return static_cast<uint32_t>(GetCurrentProcId()); On 2015/08/05 19:52:26, danakj wrote: > Is this branch basically for the browser process/other OS? Can we just call > SetUniqueIdForProcess always, and this always returns the set id? Yeah, I think it ends up being just the browser process and other OSes. I was afraid to require SetUniqueIdForProcess to be called because there are still some situations where we might forget to call it - for example, there are tests which fork, and there is also code which may call GetUniqueIdForProcess, but goes through a different main() function than chrome (tests are also an example of this). https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.cc:34: g_unique_id = unique_id; On 2015/08/05 19:52:26, danakj wrote: > can you DCHECK_GT(unique_id, 0) ? Done. https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.h:8: #include <stdint.h> On 2015/08/05 19:52:26, danakj wrote: > You want base/basictypes.h, instead of these. Is that still the case? https://code.google.com/p/chromium/codesearch#chromium/src/base/basictypes.h&... seems to suggest using stdint instead. https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.h:39: const uint32_t kInvalidUniqueId = 0; On 2015/08/05 19:52:26, danakj wrote: > Does this need to be in the header? Can it move to the .cc file? Does other code > need to use this? Don't think so, moved it into the anon namespace in the cc instead. https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.h:42: // Note that on some platforms, this is not guaranteed to be unique across On 2015/08/05 19:52:26, danakj wrote: > Who would ever want to use this one? GetCurrentProcId is still useful for APIs that need a real process ID and not a unique identifier - for example, kill or wait*. https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.h:67: // Note that on some platforms, this is not guaranteed to be unique across On 2015/08/05 19:52:26, danakj wrote: > Does this mean Process:Pid() can be non-unique (always 1) as well? Can we fix > it? It does, but as mentioned above, there are situations where we want the real PID, even if it's not unique. That's why we opted to introduce a new unique ID instead of replacing PIDs. https://codereview.chromium.org/1186873006/diff/160001/content/zygote/zygote_... File content/zygote/zygote_linux.cc (right): https://codereview.chromium.org/1186873006/diff/160001/content/zygote/zygote_... content/zygote/zygote_linux.cc:426: #if defined(OS_LINUX) On 2015/08/05 19:52:26, danakj wrote: > btw.. this file is zygote_linux.cc why does it have #if defined(OS_LINUX) as > well? From looking at filename_rules.gypi, it seems like _linux.cc files are included on openbsd/freebsd (crazy). I'd be pretty surprised if most of the other code in this file would compile/run under those OSes though, so this might just be a historical artifact.
Thanks for the comments - sorry about the slow response - just got back from some traveling.
On 2015/08/13 23:16:55, rickyz wrote: > From looking at filename_rules.gypi, it seems like _linux.cc files are included > on openbsd/freebsd (crazy). I'd be pretty surprised if most of the other code in > this file would compile/run under those OSes though, so this might just be a > historical artifact. I'm not familiar with the FreeBSD port, but the OpenBSD port relies on a bunch of patches maintained externally (http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/www/chromium/patches/). It definitely uses a lot of _linux.cc files for expediency. I wouldn't worry too much about breaking it. Usually every Chromium release, the OpenBSD maintainer goes through and fixes up the patch set to keep compiling. If you remove #ifdefs and they're actually needed, they'll probably just get re-added on the OpenBSD side.
ricky, ping? I would love to see this clean-up wrapped up.
On 2015/09/11 at 02:36:01, jln wrote: > ricky, ping? I would love to see this clean-up wrapped up. Friendly ping to any interested reviewers :-)
Sorry I didn't see that you'd replied to the comments :x https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... File base/process/process_handle.h (right): https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.h:67: // Note that on some platforms, this is not guaranteed to be unique across On 2015/08/13 23:16:55, rickyz wrote: > On 2015/08/05 19:52:26, danakj wrote: > > Does this mean Process:Pid() can be non-unique (always 1) as well? Can we fix > > it? > It does, but as mentioned above, there are situations where we want the real > PID, even if it's not unique. That's why we opted to introduce a new unique ID > instead of replacing PIDs. i think i'm not understanding something: if you're in a sandbox and your PID is reported as 1, does using PID 1 to the kernel actually work? like if you killed 1, it would kill yourself?
On 2015/09/15 18:06:39, danakj wrote: > Sorry I didn't see that you'd replied to the comments :x > > https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... > File base/process/process_handle.h (right): > > https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... > base/process/process_handle.h:67: // Note that on some platforms, this is not > guaranteed to be unique across > On 2015/08/13 23:16:55, rickyz wrote: > > On 2015/08/05 19:52:26, danakj wrote: > > > Does this mean Process:Pid() can be non-unique (always 1) as well? Can we > fix > > > it? > > It does, but as mentioned above, there are situations where we want the real > > PID, even if it's not unique. That's why we opted to introduce a new unique ID > > instead of replacing PIDs. > > i think i'm not understanding something: if you're in a sandbox and your PID is > reported as 1, does using PID 1 to the kernel actually work? like if you killed > 1, it would kill yourself? Yes! It's crucial that stuff like kill(getpid(), ..) works. Which is also why we can't "lie" by having getpid() return the "external" PID.
https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... base/process/process_handle.cc:23: return static_cast<uint32_t>(GetCurrentProcId()); On 2015/08/13 23:16:55, rickyz wrote: > On 2015/08/05 19:52:26, danakj wrote: > > Is this branch basically for the browser process/other OS? Can we just call > > SetUniqueIdForProcess always, and this always returns the set id? > > Yeah, I think it ends up being just the browser process and other OSes. I was > afraid to require SetUniqueIdForProcess to be called because there are still > some situations where we might forget to call it - for example, there are tests > which fork, and there is also code which may call GetUniqueIdForProcess, but > goes through a different main() function than chrome (tests are also an example > of this). This still really bothers me :/ It feels very unclear as a caller what I can expect here. Sometimes I can take this and use it as a PID sometimes I can't. Why can't you just DCHECK that SetUniqueIdForProcess was called before calling this. Then if any tests that you're worried about use this incorrectly, they'll DCHECK and be easily fixable. I really hate having warts in production code that is just for tests.
On 2015/09/15 18:36:32, danakj wrote: > https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... > File base/process/process_handle.cc (right): > > https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... > base/process/process_handle.cc:23: return > static_cast<uint32_t>(GetCurrentProcId()); > On 2015/08/13 23:16:55, rickyz wrote: > > On 2015/08/05 19:52:26, danakj wrote: > > > Is this branch basically for the browser process/other OS? Can we just call > > > SetUniqueIdForProcess always, and this always returns the set id? > > > > Yeah, I think it ends up being just the browser process and other OSes. I was > > afraid to require SetUniqueIdForProcess to be called because there are still > > some situations where we might forget to call it - for example, there are > tests > > which fork, and there is also code which may call GetUniqueIdForProcess, but > > goes through a different main() function than chrome (tests are also an > example > > of this). > > This still really bothers me :/ It feels very unclear as a caller what I can > expect here. Sometimes I can take this and use it as a PID sometimes I can't. > > Why can't you just DCHECK that SetUniqueIdForProcess was called before calling > this. Then if any tests that you're worried about use this incorrectly, they'll > DCHECK and be easily fixable. > > I really hate having warts in production code that is just for tests. I didn't re-read the full conversation, but I took another quick look at the CL and it does look reasonable to me. Currently a lot of code is using GetCurrentProcId() when what they really want is to get a system-wide unique id. It's a bug on Linux where ProcId are not unique process identifier (different processes will see the process id of a given process A as being different). Do we all agree we need to fix this? Is the current concern mostly about the need to implement a SetUniqueIdForProcess() (and call it!) in order to be able to make GetUniqueIdForProcess() work? It's painful, but absent another suggestion, I believe we should still do it. It feels much worse to me to rely on hacks like this: https://codereview.chromium.org/1186933004/diff/20001/content/child/blink_pla...
On Tue, Sep 22, 2015 at 11:26 AM, <jln@chromium.org> wrote: > On 2015/09/15 18:36:32, danakj wrote: > > > https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... > >> File base/process/process_handle.cc (right): >> > > > > https://codereview.chromium.org/1186873006/diff/160001/base/process/process_h... > >> base/process/process_handle.cc:23: return >> static_cast<uint32_t>(GetCurrentProcId()); >> On 2015/08/13 23:16:55, rickyz wrote: >> > On 2015/08/05 19:52:26, danakj wrote: >> > > Is this branch basically for the browser process/other OS? Can we just >> > call > >> > > SetUniqueIdForProcess always, and this always returns the set id? >> > >> > Yeah, I think it ends up being just the browser process and other OSes. >> I >> > was > >> > afraid to require SetUniqueIdForProcess to be called because there are >> still >> > some situations where we might forget to call it - for example, there >> are >> tests >> > which fork, and there is also code which may call >> GetUniqueIdForProcess, but >> > goes through a different main() function than chrome (tests are also an >> example >> > of this). >> > > This still really bothers me :/ It feels very unclear as a caller what I >> can >> expect here. Sometimes I can take this and use it as a PID sometimes I >> can't. >> > > Why can't you just DCHECK that SetUniqueIdForProcess was called before >> calling >> this. Then if any tests that you're worried about use this incorrectly, >> > they'll > >> DCHECK and be easily fixable. >> > > I really hate having warts in production code that is just for tests. >> > > I didn't re-read the full conversation, but I took another quick look at > the CL > and it does look reasonable to me. Currently a lot of code is using > GetCurrentProcId() when what they really want is to get a system-wide > unique id. > It's a bug on Linux where ProcId are not unique process identifier > (different > processes will see the process id of a given process A as being different). > Do we all agree we need to fix this? > Ya, I agree with that. > Is the current concern mostly about the need to implement a > SetUniqueIdForProcess() (and call it!) in order to be able to make > GetUniqueIdForProcess() work? It's painful, but absent another suggestion, > I > believe we should still do it. > Right, I want the "fake id namespace" to be exactly that, not sometimes a real pid sometimes it's not. Just DCHECK that something has been set and always have it be a fake chrome-created id. > > It feels much worse to me to rely on hacks like this: > > https://codereview.chromium.org/1186933004/diff/20001/content/child/blink_pla... > > https://codereview.chromium.org/1186873006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
danakj@chromium.org changed reviewers: - willchan@chromium.org
On 2015/09/22 18:38:15, danakj wrote: > > > Is the current concern mostly about the need to implement a > > SetUniqueIdForProcess() (and call it!) in order to be able to make > > GetUniqueIdForProcess() work? It's painful, but absent another suggestion, > > I > > believe we should still do it. > > > > Right, I want the "fake id namespace" to be exactly that, not sometimes a > real pid sometimes it's not. Just DCHECK that something has been set and > always have it be a fake chrome-created id. So, the question is whether or not to always require calling SetUniqueIdForProcess()? (TL;DR: for the following: PIDs not being unique is currently a Linux-only hack required by sandboxing. We should not have that complexity bleed into all platforms / all processes) If we do require always calling SetUniqueIdForProcess(), then we'll either: 1. End-up having a lot of calls to SetUniqueIdForProcess(GetProcessId()), which will look weird (but is the correct thing to do in a majority of cases). Whenever someone creates a new process type, they'll have to call SetUniqueIdForProcess manually and think hard for whether or not calling SetUniqueIdForProcess(GetProcessId()) is the right thing to do. 2. Have to change the return type of GetUniqueIdForProcess() to be very large (> 64 bits) so that we can implement a stateless way to get a unique ID by simply using a random number 3. Always require an implementation a complex stateful system to track and give unique IDs for processes (3) would be outside of the scope of base/ and it would be strange to require that complexity for any user of base. (2) would be a pretty large and painful change. (1) would add complexity where currently there is none. For most cases and most platforms, simply calling GetUniqueIdForProcess() (and having the implementation rely on the system to provide unique PIDs) is the right thing to do. I don't think we should require anyone creating a new process type / a new main using main() to have to call SetUniqueIdForProcess(GetProcessId()) and think about such oddities. I prefer the approach where only the code that tinkers with PIDs and already has to deal with the complexity of cross-namespaces PIDs and PID translation has to make an explicit call to SetUniqueIdForProcess(). That being said, I understand your concern and I agree that it's not really clear currently who has to call SetUniqueIdForProcess(). That should be better documented. Maybe the API should be renamed (and Linux-only for now). Any suggestion? SetUniqueIdForProcessBecauseImissedWithPids()? :) What do you think?
> On 2015/09/22 18:38:15, danakj wrote: > > Right, I want the "fake id namespace" to be exactly that, not sometimes a > > real pid sometimes it's not. Just DCHECK that something has been set and > > always have it be a fake chrome-created id. My hope was that it'd be very clear to callers that they can make no assumptions about the unique ID, other than that it is some opaque value that is unique between processes. If we make that extremely clear, there should hopefully not be any confusion about how the value can be used.
On Tue, Sep 22, 2015 at 12:31 PM, <jln@chromium.org> wrote: > On 2015/09/22 18:38:15, danakj wrote: > > > > Is the current concern mostly about the need to implement a >> > SetUniqueIdForProcess() (and call it!) in order to be able to make >> > GetUniqueIdForProcess() work? It's painful, but absent another >> suggestion, >> > I >> > believe we should still do it. >> > >> > > Right, I want the "fake id namespace" to be exactly that, not sometimes a >> real pid sometimes it's not. Just DCHECK that something has been set and >> always have it be a fake chrome-created id. >> > > So, the question is whether or not to always require calling > SetUniqueIdForProcess()? > > (TL;DR: for the following: PIDs not being unique is currently a Linux-only > hack > required by sandboxing. We should not have that complexity bleed into all > platforms / all processes) > > If we do require always calling SetUniqueIdForProcess(), then we'll either: > > 1. End-up having a lot of calls to SetUniqueIdForProcess(GetProcessId()), > which > will look weird (but is the correct thing to do in a majority of cases). > Whenever someone creates a new process type, they'll have to call > SetUniqueIdForProcess manually and think hard for whether or not calling > SetUniqueIdForProcess(GetProcessId()) is the right thing to do. > 2. Have to change the return type of GetUniqueIdForProcess() to be very > large (> > 64 bits) so that we can implement a stateless way to get a unique ID by > simply > using a random number > 3. Always require an implementation a complex stateful system to track and > give > unique IDs for processes > > (3) would be outside of the scope of base/ and it would be strange to > require > that complexity for any user of base. (2) would be a pretty large and > painful > change. > > (1) would add complexity where currently there is none. For most cases and > most > platforms, simply calling GetUniqueIdForProcess() (and having the > implementation > rely on the system to provide unique PIDs) is the right thing to do. I > don't > think we should require anyone creating a new process type / a new main > using > main() to have to call SetUniqueIdForProcess(GetProcessId()) and think > about > such oddities. > > I prefer the approach where only the code that tinkers with PIDs and > already has > to deal with the complexity of cross-namespaces PIDs and PID translation > has to > make an explicit call to SetUniqueIdForProcess(). > > That being said, I understand your concern and I agree that it's not really > clear currently who has to call SetUniqueIdForProcess(). That should be > better > documented. Maybe the API should be renamed (and Linux-only for now). Any > suggestion? SetUniqueIdForProcessBecauseImissedWithPids()? :) > > What do you think? > Thanks, I see what you're saying. Would it work to do these two things? 1. Let's give SetUniqueIdForProcess a scarier name. I don't hate what you just proposed really :) SetUniqueIdForProcessSincePIDsHaveChanged()? Something scary. 2. Can GetUniqueIdForProcess() return pid+1? Would that work? Then you couldn't use it as a pid blindly. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/09/22 21:16:42, danakj wrote: > On Tue, Sep 22, 2015 at 12:31 PM, <mailto:jln@chromium.org> wrote: > > > On 2015/09/22 18:38:15, danakj wrote: > > > > > > > Is the current concern mostly about the need to implement a > >> > SetUniqueIdForProcess() (and call it!) in order to be able to make > >> > GetUniqueIdForProcess() work? It's painful, but absent another > >> suggestion, > >> > I > >> > believe we should still do it. > >> > > >> > > > > Right, I want the "fake id namespace" to be exactly that, not sometimes a > >> real pid sometimes it's not. Just DCHECK that something has been set and > >> always have it be a fake chrome-created id. > >> > > > > So, the question is whether or not to always require calling > > SetUniqueIdForProcess()? > > > > (TL;DR: for the following: PIDs not being unique is currently a Linux-only > > hack > > required by sandboxing. We should not have that complexity bleed into all > > platforms / all processes) > > > > If we do require always calling SetUniqueIdForProcess(), then we'll either: > > > > 1. End-up having a lot of calls to SetUniqueIdForProcess(GetProcessId()), > > which > > will look weird (but is the correct thing to do in a majority of cases). > > Whenever someone creates a new process type, they'll have to call > > SetUniqueIdForProcess manually and think hard for whether or not calling > > SetUniqueIdForProcess(GetProcessId()) is the right thing to do. > > 2. Have to change the return type of GetUniqueIdForProcess() to be very > > large (> > > 64 bits) so that we can implement a stateless way to get a unique ID by > > simply > > using a random number > > 3. Always require an implementation a complex stateful system to track and > > give > > unique IDs for processes > > > > (3) would be outside of the scope of base/ and it would be strange to > > require > > that complexity for any user of base. (2) would be a pretty large and > > painful > > change. > > > > (1) would add complexity where currently there is none. For most cases and > > most > > platforms, simply calling GetUniqueIdForProcess() (and having the > > implementation > > rely on the system to provide unique PIDs) is the right thing to do. I > > don't > > think we should require anyone creating a new process type / a new main > > using > > main() to have to call SetUniqueIdForProcess(GetProcessId()) and think > > about > > such oddities. > > > > I prefer the approach where only the code that tinkers with PIDs and > > already has > > to deal with the complexity of cross-namespaces PIDs and PID translation > > has to > > make an explicit call to SetUniqueIdForProcess(). > > > > That being said, I understand your concern and I agree that it's not really > > clear currently who has to call SetUniqueIdForProcess(). That should be > > better > > documented. Maybe the API should be renamed (and Linux-only for now). Any > > suggestion? SetUniqueIdForProcessBecauseImissedWithPids()? :) > > > > What do you think? > > > > Thanks, I see what you're saying. Would it work to do these two things? > > 1. Let's give SetUniqueIdForProcess a scarier name. I don't hate what you > just proposed really :) SetUniqueIdForProcessSincePIDsHaveChanged()? > Something scary. > > 2. Can GetUniqueIdForProcess() return pid+1? Would that work? Then you > couldn't use it as a pid blindly. This seems good. How about going even farther and having unique IDs be something such as: 0x7F000000 ^ PID? That way, if people need to be able to do a quick mapping from unique ID to (likely) PID in a debug environment they can do it, but the unique ID can not be used as a PID (and in practice is fairly unlikely to be the PID of any existing process). Ricky would, Dana's suggestions work for you?
On 2015/09/22 at 21:24:44, jln wrote: > This seems good. How about going even farther and having unique IDs be something such as: 0x7F000000 ^ PID? > > That way, if people need to be able to do a quick mapping from unique ID to (likely) PID in a debug environment they can do it, but the unique ID can not be used > as a PID (and in practice is fairly unlikely to be the PID of any existing process). > > Ricky would, Dana's suggestions work for you? Sorry for the slowness, I've changed up the function for initializing the unique ID and mangled the process IDs by adding 1000000000 so that PIDs are still readable when IDs are printed in decimal. How does this look?
LG, 2 things https://codereview.chromium.org/1186873006/diff/240001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/240001/base/process/process_h... base/process/process_handle.cc:14: // This is never a valid mangled process ID. This is getting kinda magical and hard to really believe. Maybe just use a bool and set it true when you set the unique_id? That'd be much safer. https://codereview.chromium.org/1186873006/diff/240001/base/process/process_h... base/process/process_handle.cc:25: // Add a large power of 10 so that the pid is still the pid is still readable "the pid is still the pid is still readable" "the pid is still readable inside the mangled id"?
Thanks! https://codereview.chromium.org/1186873006/diff/240001/base/process/process_h... File base/process/process_handle.cc (right): https://codereview.chromium.org/1186873006/diff/240001/base/process/process_h... base/process/process_handle.cc:14: // This is never a valid mangled process ID. On 2015/09/23 at 21:56:32, danakj wrote: > This is getting kinda magical and hard to really believe. Maybe just use a bool and set it true when you set the unique_id? That'd be much safer. Good idea, done https://codereview.chromium.org/1186873006/diff/240001/base/process/process_h... base/process/process_handle.cc:25: // Add a large power of 10 so that the pid is still the pid is still readable On 2015/09/23 at 21:56:31, danakj wrote: > "the pid is still the pid is still readable" > > "the pid is still readable inside the mangled id"? Done
Sorry got sick and disappeared for a bit. LGTM
The CQ bit was checked by rickyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1186873006/#ps280001 (title: "Rebase")
Ah, no worries, I've been quite distracted lately too, cqing
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1186873006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1186873006/280001
Message was sent while issue was closed.
Committed patchset #13 (id:280001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/5b937a380101191cdd460033f4b72394b6638798 Cr-Commit-Position: refs/heads/master@{#355440} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
