Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18192:
Implemented process_util_mac.mm because net_unittests requires those function... (Closed)
|
Created:
11 years, 11 months ago by honten.org Modified:
5 years, 1 month ago CC:
chromium-reviews_googlegroups.com Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionImplemented process_util_mac.mm because net_unittests requires those functions.
Also I added some wait value right after HTTP server was lauched.
Because request url test failed after the launch withouht wait.
I also change kHTTPDefaultPort number because there is confliction in my Mac OS X enviroment.(only for mac)
BUG=3661
Patch Set 1 #
Total comments: 23
Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 48
Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 56
Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 6
Patch Set 12 : '' #
Total comments: 9
Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #
Messages
Total messages: 29 (0 generated)
Hi, My name is honten. This e-mail is retry because my review process was wrong ;-) I want to try to help Mac version bug fixing and implementation. I try to start to fix 3661, because it's much easier to understand an to compare the function with Windows implementation. At first, I made patch for process_mac_util.mm because I found "not implemeted" message while I'm running net_unittests. After I finish the implementaion, I run base_unittests. It looks fine. (Actually I want to try to fix UI bugs but it's hard to understand what is bug from Wiki and bug report...)
Please fix lint errors. http://codereview.chromium.org/18192/diff/1/2 File net/url_request/url_request_unittest.h (right): http://codereview.chromium.org/18192/diff/1/2#newcode33 Line 33: const int kHTTPDefaultPort = 1338; If it is already used; then why are you still using 1338. http://codereview.chromium.org/18192/diff/1/2#newcode444 Line 444: PlatformThread::Sleep(1000); Don't wait any case. Enclose it with #ifdef
Hi Honten, I really appreciate the patch contribution. So far I have only reviewed the style. I know it might seem a bit annoying, but we try to follow the Google C++ style guidelines very closely. You have a few places where your indention is off (we only use 2-space indention, or 4-space for a line continuation, never tabs or bigger spaces). You also have some lines that are over 80 chars, etc. Once the style is cleaned up a bit, I will review the actual code. Thanks again! -- dean http://codereview.chromium.org/18192/diff/1/4 File base/process_util.h (right): http://codereview.chromium.org/18192/diff/1/4#newcode219 Line 219: kinfo_proc* kinfo_proc_; Is it safe to forward declare kinfo_proc? Then you would only need to include the header in the .cc file. I imagine process_utils could get pulled into a lot of places, so it's nice to cut down on unneeded includes. Not a big deal though. http://codereview.chromium.org/18192/diff/1/3 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/1/3#newcode10 Line 10: #include "base/platform_thread.h" We sort our includes alphabetically. http://codereview.chromium.org/18192/diff/1/3#newcode44 Line 44: You added extra end of line whitespace here. We don't allow any whitespace at end of lines. http://codereview.chromium.org/18192/diff/1/3#newcode102 Line 102: // just copy fomr process_util_linux.cc Typo "from". Additionally, all of our comments are in the form: This is a comment. The capitalization and period are important. http://codereview.chromium.org/18192/diff/1/3#newcode112 Line 112: return (signum == SIGSEGV || signum == SIGILL || signum == SIGABRT || signum == SIGFPE); Lines are required to be under 80 characters long. I would additionally probably have written this as a switch(). http://codereview.chromium.org/18192/diff/1/3#newcode117 Line 117: return (exitcode != 0); No ()s around a return. This could just be: return WEXITSTATUS(status) != 0; http://codereview.chromium.org/18192/diff/1/3#newcode124 Line 124: ///////////////////////////////// start to implement NamedProcessIterator We don't do comment banners like this. http://codereview.chromium.org/18192/diff/1/3#newcode128 Line 128: executable_name_(executable_name), This should be pulled up on the same line as the : http://codereview.chromium.org/18192/diff/1/3#newcode131 Line 131: index_of_kinfo_proc_(0), Your indention is weird here (tabs?) http://codereview.chromium.org/18192/diff/1/3#newcode133 Line 133: { This should be { } on the same line as filter_ http://codereview.chromium.org/18192/diff/1/3#newcode159 Line 159: kinfo_proc_ = (struct kinfo_proc *)malloc(len); we use c++ style cases only (static_cast<>, etc) http://codereview.chromium.org/18192/diff/1/3#newcode181 Line 181: if((&kinfo_proc_[index_of_kinfo_proc_])->kp_proc.p_stat != SZOMB) { Is this extra & -> neccessary? Could it not be: kinfo_proc_[index_of_kinfo_proc_].kp_proc ? http://codereview.chromium.org/18192/diff/1/3#newcode184 Line 184: int mib[4] = { KERN_PROCARGS, KERN_PROCARGS, kinfo->kp_proc.p_pid, 0}; no space after { http://codereview.chromium.org/18192/diff/1/3#newcode185 Line 185: char *proc_argv(NULL); * goes on the type. http://codereview.chromium.org/18192/diff/1/3#newcode191 Line 191: } Your indention has gone all weird here, tabs? http://codereview.chromium.org/18192/diff/1/3#newcode213 Line 213: if(index != 0) {// beak only if there is valid name always two spaces before any // comments. http://codereview.chromium.org/18192/diff/1/3#newcode227 Line 227: int len = exec_name.size() + 1; maybe just use std::min() ? http://codereview.chromium.org/18192/diff/1/3#newcode238 Line 238: bool result = strcmp(WideToASCII(executable_name_).c_str(), entry_.szExeFile) == 0 && 80 cols. Also, it is probably better to use the std::string comparisons that going to a c_str() with strcmp(). http://codereview.chromium.org/18192/diff/1/3#newcode271 Line 271: base::Time end_time = base::Time::Now() + base::TimeDelta::FromMilliseconds(wait_milliseconds); 80 cols
Thank you for your review. I want to try to fix it. Thanks, On 2009/01/19 12:36:55, Dean McNamee wrote: > Hi Honten, > > I really appreciate the patch contribution. So far I have only reviewed the > style. I know it might seem a bit annoying, but we try to follow the Google C++ > style guidelines very closely. > > You have a few places where your indention is off (we only use 2-space > indention, or 4-space for a line continuation, never tabs or bigger spaces). > You also have some lines that are over 80 chars, etc. > > Once the style is cleaned up a bit, I will review the actual code. > > Thanks again! > -- dean > > http://codereview.chromium.org/18192/diff/1/4 > File base/process_util.h (right): > > http://codereview.chromium.org/18192/diff/1/4#newcode219 > Line 219: kinfo_proc* kinfo_proc_; > Is it safe to forward declare kinfo_proc? Then you would only need to include > the header in the .cc file. I imagine process_utils could get pulled into a lot > of places, so it's nice to cut down on unneeded includes. Not a big deal > though. > > http://codereview.chromium.org/18192/diff/1/3 > File base/process_util_mac.mm (right): > > http://codereview.chromium.org/18192/diff/1/3#newcode10 > Line 10: #include "base/platform_thread.h" > We sort our includes alphabetically. > > http://codereview.chromium.org/18192/diff/1/3#newcode44 > Line 44: > You added extra end of line whitespace here. We don't allow any whitespace at > end of lines. > > http://codereview.chromium.org/18192/diff/1/3#newcode102 > Line 102: // just copy fomr process_util_linux.cc > Typo "from". Additionally, all of our comments are in the form: > > This is a comment. > > The capitalization and period are important. > > http://codereview.chromium.org/18192/diff/1/3#newcode112 > Line 112: return (signum == SIGSEGV || signum == SIGILL || signum == SIGABRT || > signum == SIGFPE); > Lines are required to be under 80 characters long. I would additionally > probably have written this as a switch(). > > http://codereview.chromium.org/18192/diff/1/3#newcode117 > Line 117: return (exitcode != 0); > No ()s around a return. This could just be: > > return WEXITSTATUS(status) != 0; > > http://codereview.chromium.org/18192/diff/1/3#newcode124 > Line 124: ///////////////////////////////// start to implement > NamedProcessIterator > We don't do comment banners like this. > > http://codereview.chromium.org/18192/diff/1/3#newcode128 > Line 128: executable_name_(executable_name), > This should be pulled up on the same line as the : > > http://codereview.chromium.org/18192/diff/1/3#newcode131 > Line 131: index_of_kinfo_proc_(0), > Your indention is weird here (tabs?) > > http://codereview.chromium.org/18192/diff/1/3#newcode133 > Line 133: { > This should be { } on the same line as filter_ > > http://codereview.chromium.org/18192/diff/1/3#newcode159 > Line 159: kinfo_proc_ = (struct kinfo_proc *)malloc(len); > we use c++ style cases only (static_cast<>, etc) > > http://codereview.chromium.org/18192/diff/1/3#newcode181 > Line 181: if((&kinfo_proc_[index_of_kinfo_proc_])->kp_proc.p_stat != SZOMB) { > Is this extra & -> neccessary? Could it not be: > kinfo_proc_[index_of_kinfo_proc_].kp_proc ? > > http://codereview.chromium.org/18192/diff/1/3#newcode184 > Line 184: int mib[4] = { KERN_PROCARGS, KERN_PROCARGS, kinfo->kp_proc.p_pid, 0}; > no space after { > > http://codereview.chromium.org/18192/diff/1/3#newcode185 > Line 185: char *proc_argv(NULL); > * goes on the type. > > http://codereview.chromium.org/18192/diff/1/3#newcode191 > Line 191: } > Your indention has gone all weird here, tabs? > > http://codereview.chromium.org/18192/diff/1/3#newcode213 > Line 213: if(index != 0) {// beak only if there is valid name > always two spaces before any // comments. > > http://codereview.chromium.org/18192/diff/1/3#newcode227 > Line 227: int len = exec_name.size() + 1; > maybe just use std::min() ? > > http://codereview.chromium.org/18192/diff/1/3#newcode238 > Line 238: bool result = strcmp(WideToASCII(executable_name_).c_str(), > entry_.szExeFile) == 0 && > 80 cols. Also, it is probably better to use the std::string comparisons that > going to a c_str() with strcmp(). > > http://codereview.chromium.org/18192/diff/1/3#newcode271 > Line 271: base::Time end_time = base::Time::Now() + > base::TimeDelta::FromMilliseconds(wait_milliseconds); > 80 cols
http://codereview.chromium.org/18192/diff/1/3 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/1/3#newcode238 Line 238: bool result = strcmp(WideToASCII(executable_name_).c_str(), entry_.szExeFile) == 0 && On 2009/01/19 12:36:56, Dean McNamee wrote: > 80 cols. Also, it is probably better to use the std::string comparisons that > going to a c_str() with strcmp(). I agree with you. But this code comes from process_util_linux.cc. Should I change both ? http://codereview.chromium.org/18192/diff/1/3#newcode271 Line 271: base::Time end_time = base::Time::Now() + base::TimeDelta::FromMilliseconds(wait_milliseconds); On 2009/01/19 12:36:56, Dean McNamee wrote: > 80 cols Also it comes from linux code.
Now I follow the comment except 80 cols. What do you think ? On 2009/01/20 07:40:27, honten wrote: > http://codereview.chromium.org/18192/diff/1/3 > File base/process_util_mac.mm (right): > > http://codereview.chromium.org/18192/diff/1/3#newcode238 > Line 238: bool result = strcmp(WideToASCII(executable_name_).c_str(), > entry_.szExeFile) == 0 && > On 2009/01/19 12:36:56, Dean McNamee wrote: > > 80 cols. Also, it is probably better to use the std::string comparisons that > > going to a c_str() with strcmp(). > > I agree with you. > > But this code comes from process_util_linux.cc. > Should I change both ? > > http://codereview.chromium.org/18192/diff/1/3#newcode271 > Line 271: base::Time end_time = base::Time::Now() + > base::TimeDelta::FromMilliseconds(wait_milliseconds); > On 2009/01/19 12:36:56, Dean McNamee wrote: > > 80 cols > > Also it comes from linux code.
I am trying to address some of the issues in the old Linux code right now, I will send out a CL. Thanks On 2009/01/21 07:53:40, honten wrote: > Now I follow the comment except 80 cols. > > What do you think ? > > On 2009/01/20 07:40:27, honten wrote: > > http://codereview.chromium.org/18192/diff/1/3 > > File base/process_util_mac.mm (right): > > > > http://codereview.chromium.org/18192/diff/1/3#newcode238 > > Line 238: bool result = strcmp(WideToASCII(executable_name_).c_str(), > > entry_.szExeFile) == 0 && > > On 2009/01/19 12:36:56, Dean McNamee wrote: > > > 80 cols. Also, it is probably better to use the std::string comparisons > that > > > going to a c_str() with strcmp(). > > > > I agree with you. > > > > But this code comes from process_util_linux.cc. > > Should I change both ? > > > > http://codereview.chromium.org/18192/diff/1/3#newcode271 > > Line 271: base::Time end_time = base::Time::Now() + > > base::TimeDelta::FromMilliseconds(wait_milliseconds); > > On 2009/01/19 12:36:56, Dean McNamee wrote: > > > 80 cols > > > > Also it comes from linux code.
I just committed r8362 and r8363. The first one fixed some of the style problems in process_util_linux. The second moved DidProcessCrash() to process_util_posix, so that the code is shared between Linux and Mac. Let me know when you have uploaded a new revision merged to the newest changes. Thanks! On 2009/01/21 12:41:58, Dean McNamee wrote: > I am trying to address some of the issues in the old Linux code right now, I > will send out a CL. > > Thanks > > On 2009/01/21 07:53:40, honten wrote: > > Now I follow the comment except 80 cols. > > > > What do you think ? > > > > On 2009/01/20 07:40:27, honten wrote: > > > http://codereview.chromium.org/18192/diff/1/3 > > > File base/process_util_mac.mm (right): > > > > > > http://codereview.chromium.org/18192/diff/1/3#newcode238 > > > Line 238: bool result = strcmp(WideToASCII(executable_name_).c_str(), > > > entry_.szExeFile) == 0 && > > > On 2009/01/19 12:36:56, Dean McNamee wrote: > > > > 80 cols. Also, it is probably better to use the std::string comparisons > > that > > > > going to a c_str() with strcmp(). > > > > > > I agree with you. > > > > > > But this code comes from process_util_linux.cc. > > > Should I change both ? > > > > > > http://codereview.chromium.org/18192/diff/1/3#newcode271 > > > Line 271: base::Time end_time = base::Time::Now() + > > > base::TimeDelta::FromMilliseconds(wait_milliseconds); > > > On 2009/01/19 12:36:56, Dean McNamee wrote: > > > > 80 cols > > > > > > Also it comes from linux code.
I hope that we never ever need this API. That's not to say that the code is bad - thank you very much for implementing it! What I mean is that the idea itself seems really flawed to me. I don't think that any process has any business looking up other processes by name and killing them. That kind of thing might make sense in the Windows world, but it doesn't make any sense to my POSIX-oriented mind. Initial comments follow. Thanks again for the patch. http://codereview.chromium.org/18192/diff/28/30 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/28/30#newcode7 Line 7: #include "base/platform_thread.h" Whoa whoa whoa! Correct #include order would be: #include "base/process_util.h" // header for this impl. file // blank line #import <Cocoa/Cocoa.h> // system-provided C headers #include <spawn.h> // [...] #include <sys/wait.h> // blank line #include <string> // system-provided C++ headers // blank line #include "base/logging.h" // [...] #include "base/time.h" See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... (and the rest of that document). http://codereview.chromium.org/18192/diff/28/30#newcode50 Line 50: No need to introduce whitespace here. http://codereview.chromium.org/18192/diff/28/30#newcode103 Line 103: // Just copy from process_util_linux.cc. (Ignoring this for now, pending your refactor after Dean's checkin...will revisit when revised...) http://codereview.chromium.org/18192/diff/28/30#newcode125 Line 125: // Start to implement NamedProcessIterator. This comment is pointless. Does it mean that the implementation is incomplete? If so, it should be a TODO (see the style guide) identifying what aspects need work. If the comment just means "here's the implementation," omit it. http://codereview.chromium.org/18192/diff/28/30#newcode127 Line 127: const ProcessFilter* filter) : Refer to http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia... for proper formatting. http://codereview.chromium.org/18192/diff/28/30#newcode148 Line 148: int mib[4]; Prefer initialization at the declaration site rather than splitting it all up (and then you can omit the 4, too): int mib[] = { CTL_KERN, KERN_PROC, KERN_PROC_ALL, 0 }; http://codereview.chromium.org/18192/diff/28/30#newcode153 Line 153: size_t len(0); Odd indentation. Prefer size_t len = 0, it's a little bit more readable, and size_t is just a typedef for a native integral type anyway. http://codereview.chromium.org/18192/diff/28/30#newcode155 Line 155: if(sysctl(mib, 4, NULL, &len, NULL, 0) < 0) { Instead of 4, use arraysize(mib). http://codereview.chromium.org/18192/diff/28/30#newcode155 Line 155: if(sysctl(mib, 4, NULL, &len, NULL, 0) < 0) { if<space>(sysctl. Throughout. Rather than if (error) {/*no-op*/} else {/*do work*/}, this should check for the success condition. But are you sure that "do nothing" is the right thing to do when sysctl fails? We should probably at least have a DLOG. http://codereview.chromium.org/18192/diff/28/30#newcode156 Line 156: // error occorred, do nothing Spelling: occurred. We prefer comments that are complete sentences, with capital letters and periods and all. http://codereview.chromium.org/18192/diff/28/30#newcode159 Line 159: if(sysctl(mib, 4, kinfo_proc_, &len, NULL, 0) == 0) { The process table may have grown between the two sysctls. You might want to leave some extra room in what you malloc to account for a few extra processes. Actually, I'd recommend avoiding malloc entirely. kinfo_proc_ should be a std::vector<struct kinfo_proc> so that you can access it more traditionally (and should be renamed kinfo_procs_ to reflect its plurality). This will also let you get rid of the num_of_kinfo_proc_ variable and eliminate the malloc/free action. Something like this (add error checking and other stuff as needed): sysctl(mib, arraysize(mib), NULL, &size, NULL, 0); int elements = size / sizeof(struct kinfo_proc); elements += 16; // Leave some spare room for process table growth. size = elements * sizeof(struct kinfo_proc); kinfo_procs_.resize(elements); sysctl(mib, arraysize(mib), &kinfo_procs_[0], &size, NULL, 0); elements = size / sizeof(struct kinfo_proc); kinfo_procs_.resize(elements); http://codereview.chromium.org/18192/diff/28/30#newcode177 Line 177: kinfo_proc* kinfo(NULL); Prefer kinfo_proc* kinfo = NULL; http://codereview.chromium.org/18192/diff/28/30#newcode182 Line 182: int mib[4] = {KERN_PROCARGS, KERN_PROCARGS, kinfo->kp_proc.p_pid, 0}; 4 is unnecessary, just let the compiler count the elements for you. http://codereview.chromium.org/18192/diff/28/30#newcode183 Line 183: char* proc_argv(NULL); char* proc_argv = NULL. Same on the next line and elsewhere. I won't point these out anymore. This API gives back a flat string, right? We shouldn't call it "argv" then, because that stands for "argument vector," and we're not actually getting a vector. Call it "args" or something - even if it's not the best name, it matches KERN_PROCARGS. http://codereview.chromium.org/18192/diff/28/30#newcode186 Line 186: if(sysctl(mib, 3, NULL, &argv_len, NULL, 0) < 0) { mib[] has 4 elements but you say 3 here. Ideally, you'd use arraysize, but what's up with the discrepancy? This happens again below with the next sysctl. http://codereview.chromium.org/18192/diff/28/30#newcode191 Line 191: proc_argv = (char*)malloc(sizeof(char) * argv_len); Why don't you write into the string directly and save a copy? You also save a malloc and the bug-prone dangling free problem. In the case below, you neglected to free if the second sysctl failed. std::string args; args.reserve(args_len); args.resize(args_len - 1); // Don't include the trailing NUL sysctl(mib, arraysize(mib), &args[0], &argv_len, NULL, 0); http://codereview.chromium.org/18192/diff/28/30#newcode201 Line 201: std::string::size_type index(0); This block should have a comment. It's obvious that the code is looking for a slash but I'm not sure why or what it's really supposed to be doing. In fact, the whole slash-seeking thing seems kind of wrong, so I'm going to defer reviewing this section until you say what this is intended to do. http://codereview.chromium.org/18192/diff/28/30#newcode211 Line 211: if(index != 0) { // beak only if there is valid name Speling. http://codereview.chromium.org/18192/diff/28/30#newcode220 Line 220: } I feel like this loop can be restructured to be less deep and to not have this "return false" termination case at the bottom like this. http://codereview.chromium.org/18192/diff/28/30#newcode228 Line 228: memcpy(entry_.szExeFile, exec_name.c_str(), len - 1); Gross Windowsy API detected - good candidate for future cleanup. Can you mark a TODO in the header if one isn't already there? http://codereview.chromium.org/18192/diff/28/30#newcode229 Line 229: entry_.szExeFile[len] = 0; Don't do this, just memcpy up to len (without the - 1). (std::string).c_str() always gives back a NUL-terminated string. http://codereview.chromium.org/18192/diff/28/30#newcode236 Line 236: bool result = strcmp(WideToASCII(executable_name_).c_str(), entry_.szExeFile) == 0 && Also: 80 columns.
http://codereview.chromium.org/18192/diff/28/30 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/28/30#newcode7 Line 7: #include "base/platform_thread.h" Done. But I think original #include order is incorrect ;-) On 2009/01/21 17:48:26, Mark Mentovai wrote: > Whoa whoa whoa! > > Correct #include order would be: > > #include "base/process_util.h" // header for this impl. file > // blank line > #import <Cocoa/Cocoa.h> // system-provided C headers > #include <spawn.h> > // [...] > #include <sys/wait.h> > // blank line > #include <string> // system-provided C++ headers > // blank line > #include "base/logging.h" > // [...] > #include "base/time.h" > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... > (and the rest of that document). http://codereview.chromium.org/18192/diff/28/30#newcode7 Line 7: #include "base/platform_thread.h" On 2009/01/21 17:48:26, Mark Mentovai wrote: > Whoa whoa whoa! > > Correct #include order would be: > > #include "base/process_util.h" // header for this impl. file > // blank line > #import <Cocoa/Cocoa.h> // system-provided C headers > #include <spawn.h> > // [...] > #include <sys/wait.h> > // blank line > #include <string> // system-provided C++ headers > // blank line > #include "base/logging.h" > // [...] > #include "base/time.h" > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... > (and the rest of that document). Originally, the order was wrong. Done. http://codereview.chromium.org/18192/diff/28/30#newcode50 Line 50: Done. On 2009/01/21 17:48:26, Mark Mentovai wrote: > No need to introduce whitespace here. http://codereview.chromium.org/18192/diff/28/30#newcode125 Line 125: // Start to implement NamedProcessIterator. Removed. On 2009/01/21 17:48:26, Mark Mentovai wrote: > This comment is pointless. Does it mean that the implementation is incomplete? > If so, it should be a TODO (see the style guide) identifying what aspects need > work. If the comment just means "here's the implementation," omit it. http://codereview.chromium.org/18192/diff/28/30#newcode127 Line 127: const ProcessFilter* filter) : Done. On 2009/01/21 17:48:26, Mark Mentovai wrote: > Refer to > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Initia... > for proper formatting. http://codereview.chromium.org/18192/diff/28/30#newcode148 Line 148: int mib[4]; On 2009/01/21 17:48:26, Mark Mentovai wrote: > Prefer initialization at the declaration site rather than splitting it all up > (and then you can omit the 4, too): > > int mib[] = { CTL_KERN, KERN_PROC, KERN_PROC_ALL, 0 }; Done. http://codereview.chromium.org/18192/diff/28/30#newcode153 Line 153: size_t len(0); On 2009/01/21 17:48:26, Mark Mentovai wrote: > Odd indentation. > > Prefer size_t len = 0, it's a little bit more readable, and size_t is just a > typedef for a native integral type anyway. Done. http://codereview.chromium.org/18192/diff/28/30#newcode155 Line 155: if(sysctl(mib, 4, NULL, &len, NULL, 0) < 0) { On 2009/01/21 17:48:26, Mark Mentovai wrote: > Instead of 4, use arraysize(mib). Done. http://codereview.chromium.org/18192/diff/28/30#newcode155 Line 155: if(sysctl(mib, 4, NULL, &len, NULL, 0) < 0) { On 2009/01/21 17:48:26, Mark Mentovai wrote: > if<space>(sysctl. Throughout. > > Rather than if (error) {/*no-op*/} else {/*do work*/}, this should check for the > success condition. > > But are you sure that "do nothing" is the right thing to do when sysctl fails? > We should probably at least have a DLOG. Done. http://codereview.chromium.org/18192/diff/28/30#newcode156 Line 156: // error occorred, do nothing On 2009/01/21 17:48:26, Mark Mentovai wrote: > Spelling: occurred. > > We prefer comments that are complete sentences, with capital letters and periods > and all. Done. http://codereview.chromium.org/18192/diff/28/30#newcode159 Line 159: if(sysctl(mib, 4, kinfo_proc_, &len, NULL, 0) == 0) { On 2009/01/21 17:48:26, Mark Mentovai wrote: > The process table may have grown between the two sysctls. You might want to > leave some extra room in what you malloc to account for a few extra processes. > > Actually, I'd recommend avoiding malloc entirely. kinfo_proc_ should be a > std::vector<struct kinfo_proc> so that you can access it more traditionally (and > should be renamed kinfo_procs_ to reflect its plurality). This will also let > you get rid of the num_of_kinfo_proc_ variable and eliminate the malloc/free > action. > > Something like this (add error checking and other stuff as needed): > > sysctl(mib, arraysize(mib), NULL, &size, NULL, 0); > int elements = size / sizeof(struct kinfo_proc); > elements += 16; // Leave some spare room for process table growth. > size = elements * sizeof(struct kinfo_proc); > kinfo_procs_.resize(elements); > sysctl(mib, arraysize(mib), &kinfo_procs_[0], &size, NULL, 0); > elements = size / sizeof(struct kinfo_proc); > kinfo_procs_.resize(elements); Done. http://codereview.chromium.org/18192/diff/28/30#newcode177 Line 177: kinfo_proc* kinfo(NULL); On 2009/01/21 17:48:26, Mark Mentovai wrote: > Prefer kinfo_proc* kinfo = NULL; Done. http://codereview.chromium.org/18192/diff/28/30#newcode182 Line 182: int mib[4] = {KERN_PROCARGS, KERN_PROCARGS, kinfo->kp_proc.p_pid, 0}; On 2009/01/21 17:48:26, Mark Mentovai wrote: > 4 is unnecessary, just let the compiler count the elements for you. Done. http://codereview.chromium.org/18192/diff/28/30#newcode183 Line 183: char* proc_argv(NULL); On 2009/01/21 17:48:26, Mark Mentovai wrote: > char* proc_argv = NULL. Same on the next line and elsewhere. I won't point > these out anymore. > > This API gives back a flat string, right? We shouldn't call it "argv" then, > because that stands for "argument vector," and we're not actually getting a > vector. Call it "args" or something - even if it's not the best name, it > matches KERN_PROCARGS. Done. http://codereview.chromium.org/18192/diff/28/30#newcode183 Line 183: char* proc_argv(NULL); On 2009/01/21 17:48:26, Mark Mentovai wrote: > char* proc_argv = NULL. Same on the next line and elsewhere. I won't point > these out anymore. > > This API gives back a flat string, right? We shouldn't call it "argv" then, > because that stands for "argument vector," and we're not actually getting a > vector. Call it "args" or something - even if it's not the best name, it > matches KERN_PROCARGS. Done. http://codereview.chromium.org/18192/diff/28/30#newcode186 Line 186: if(sysctl(mib, 3, NULL, &argv_len, NULL, 0) < 0) { On 2009/01/21 17:48:26, Mark Mentovai wrote: > mib[] has 4 elements but you say 3 here. Ideally, you'd use arraysize, but > what's up with the discrepancy? This happens again below with the next sysctl. Done. http://codereview.chromium.org/18192/diff/28/30#newcode191 Line 191: proc_argv = (char*)malloc(sizeof(char) * argv_len); On 2009/01/21 17:48:26, Mark Mentovai wrote: > Why don't you write into the string directly and save a copy? You also save a > malloc and the bug-prone dangling free problem. In the case below, you > neglected to free if the second sysctl failed. > > std::string args; > args.reserve(args_len); > args.resize(args_len - 1); // Don't include the trailing NUL > sysctl(mib, arraysize(mib), &args[0], &argv_len, NULL, 0); Done. http://codereview.chromium.org/18192/diff/28/30#newcode201 Line 201: std::string::size_type index(0); On 2009/01/21 17:48:26, Mark Mentovai wrote: > This block should have a comment. It's obvious that the code is looking for a > slash but I'm not sure why or what it's really supposed to be doing. In fact, > the whole slash-seeking thing seems kind of wrong, so I'm going to defer > reviewing this section until you say what this is intended to do. Done. http://codereview.chromium.org/18192/diff/28/30#newcode211 Line 211: if(index != 0) { // beak only if there is valid name On 2009/01/21 17:48:26, Mark Mentovai wrote: > Speling. Done. http://codereview.chromium.org/18192/diff/28/30#newcode220 Line 220: } On 2009/01/21 17:48:26, Mark Mentovai wrote: > I feel like this loop can be restructured to be less deep and to not have this > "return false" termination case at the bottom like this. Done. http://codereview.chromium.org/18192/diff/28/30#newcode228 Line 228: memcpy(entry_.szExeFile, exec_name.c_str(), len - 1); On 2009/01/21 17:48:26, Mark Mentovai wrote: > Gross Windowsy API detected - good candidate for future cleanup. Can you mark a > TODO in the header if one isn't already there? Done. http://codereview.chromium.org/18192/diff/28/30#newcode229 Line 229: entry_.szExeFile[len] = 0; On 2009/01/21 17:48:26, Mark Mentovai wrote: > Don't do this, just memcpy up to len (without the - 1). (std::string).c_str() > always gives back a NUL-terminated string. The same code is used in process_util_linux.cc. Should I fix both? http://codereview.chromium.org/18192/diff/28/30#newcode236 Line 236: bool result = strcmp(WideToASCII(executable_name_).c_str(), entry_.szExeFile) == 0 && On 2009/01/21 17:48:26, Mark Mentovai wrote: > Also: 80 columns. Done.
I will leave the real review up to the Mac guys. Just some style comments: http://codereview.chromium.org/18192/diff/28/30 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/28/30#newcode50 Line 50: Extra end of line space here. http://codereview.chromium.org/18192/diff/28/30#newcode103 Line 103: // Just copy from process_util_linux.cc. Can we move this into process_util_posix then and have the two shared? http://codereview.chromium.org/18192/diff/45/218 File base/process_util.h (left): http://codereview.chromium.org/18192/diff/45/218#oldcode11 Line 11: #include "base/basictypes.h" You can't move this, it's needed for the OS_ macros (OS_WIN, OS_LINUX, etc) http://codereview.chromium.org/18192/diff/45/217 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/45/217#newcode151 Line 151: while(index_of_kinfo_proc_ < kinfo_proc_.size()) { space after while http://codereview.chromium.org/18192/diff/45/217#newcode152 Line 152: if(kinfo_proc_[index_of_kinfo_proc_].kp_proc.p_stat != SZOMB) { space after if http://codereview.chromium.org/18192/diff/45/217#newcode176 Line 176: while(true) { space after while http://codereview.chromium.org/18192/diff/45/217#newcode179 Line 179: if(index == std::string::npos) { space after if http://codereview.chromium.org/18192/diff/45/217#newcode194 Line 194: if(index_of_kinfo_proc_ >= kinfo_proc_.size()) space http://codereview.chromium.org/18192/diff/45/217#newcode202 Line 202: len = NAME_MAX; use std::min ? http://codereview.chromium.org/18192/diff/45/217#newcode211 Line 211: if (WideToASCII(executable_name_) != entry_.szExeFile) In the case of OSX, this situation might be different. Is the exe file perhaps in UTF8, and you can use WideToUTF8 and everything will work correctly?
On 2009/01/22 10:21:15, Dean McNamee wrote: > I will leave the real review up to the Mac guys. Just some style comments: > > http://codereview.chromium.org/18192/diff/28/30 > File base/process_util_mac.mm (right): > > http://codereview.chromium.org/18192/diff/28/30#newcode50 > Line 50: > Extra end of line space here. > > http://codereview.chromium.org/18192/diff/28/30#newcode103 > Line 103: // Just copy from process_util_linux.cc. > Can we move this into process_util_posix then and have the two shared? Ignore these two comments above, they were super old and somehow got sent with my new ones. > > http://codereview.chromium.org/18192/diff/45/218 > File base/process_util.h (left): > > http://codereview.chromium.org/18192/diff/45/218#oldcode11 > Line 11: #include "base/basictypes.h" > You can't move this, it's needed for the OS_ macros (OS_WIN, OS_LINUX, etc) > > http://codereview.chromium.org/18192/diff/45/217 > File base/process_util_mac.mm (right): > > http://codereview.chromium.org/18192/diff/45/217#newcode151 > Line 151: while(index_of_kinfo_proc_ < kinfo_proc_.size()) { > space after while > > http://codereview.chromium.org/18192/diff/45/217#newcode152 > Line 152: if(kinfo_proc_[index_of_kinfo_proc_].kp_proc.p_stat != SZOMB) { > space after if > > http://codereview.chromium.org/18192/diff/45/217#newcode176 > Line 176: while(true) { > space after while > > http://codereview.chromium.org/18192/diff/45/217#newcode179 > Line 179: if(index == std::string::npos) { > space after if > > http://codereview.chromium.org/18192/diff/45/217#newcode194 > Line 194: if(index_of_kinfo_proc_ >= kinfo_proc_.size()) > space > > http://codereview.chromium.org/18192/diff/45/217#newcode202 > Line 202: len = NAME_MAX; > use std::min ? > > http://codereview.chromium.org/18192/diff/45/217#newcode211 > Line 211: if (WideToASCII(executable_name_) != entry_.szExeFile) > In the case of OSX, this situation might be different. Is the exe file perhaps > in UTF8, and you can use WideToUTF8 and everything will work correctly?
http://codereview.chromium.org/18192/diff/45/218 File base/process_util.h (right): http://codereview.chromium.org/18192/diff/45/218#newcode22 Line 22: #include <vector> I wouldn't bother conditionalizing this #include, it's completely harmless on other platforms, and the #if is ugly. http://codereview.chromium.org/18192/diff/45/218#newcode225 Line 225: std::vector<kinfo_proc> kinfo_proc_; I had suggested renaming this kinfo_procs_ to indicate that it's plural. That's handy where the definition that says "vector" isn't necessarily visible. http://codereview.chromium.org/18192/diff/45/217 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/45/217#newcode123 Line 123: DLOG(ERROR) << "Failed to call sysctl."; Nit: we DID call sysctl, but IT failed. Reword the message? Same below. http://codereview.chromium.org/18192/diff/45/217#newcode125 Line 125: } else { No "else" branch needed after a return. http://codereview.chromium.org/18192/diff/45/217#newcode130 Line 130: if (sysctl(mib, arraysize(mib), &kinfo_proc_[0], &len, NULL, 0) == 0) { You can't pass &len without resetting it to be equivalent to the resized kinfo_procs_. sysctl needs to know that we're leaving extra room for process table growth. Before this line, you need: len = num_of_kinfo_procs * sizeof(struct kinfo_proc) Also, restructure this to get rid of the empty "if" branch. Finally, you should decide between whether to treat "== 0" or "any positive integer" as sysctl success. You've done a couple different things with your sysctl tests, you should be consistent. http://codereview.chromium.org/18192/diff/45/217#newcode151 Line 151: while(index_of_kinfo_proc_ < kinfo_proc_.size()) { This would be better written as: for (; index_of_kinfo_proc_ < kinfo_proc_.size(); ++index_of_kinfo_proc_) That lets you get rid of the three separate ++index_of_kinfo_proc_ statements in the loop body, and means that you don't have to remember to add another each time you add a "continue". http://codereview.chromium.org/18192/diff/45/217#newcode157 Line 157: std::string args; I would move this declaration down until you actually need it, where you start using it. http://codereview.chromium.org/18192/diff/45/217#newcode162 Line 162: //DLOG(DEBUG) << "Failed to call sysctrl."; Why are these commented out? Why does it say sysctrl (emphasis on the r)? Do we need this or not? http://codereview.chromium.org/18192/diff/45/217#newcode167 Line 167: args.resize(args_len - 1); // Don't include the trailing NULL. Style nit: two spaces before an inline comment. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem... http://codereview.chromium.org/18192/diff/45/217#newcode174 Line 174: // Get exec name from args with '/'. Once again, this section is a little bit opaque. It really needs a comment to explain what's supposed to be going on here. I think that there are bugs, but I can't tell for sure unless I know what you're trying to do. "Get exec name from args with '/'" isn't enough, can you describe how the string is to be processed? http://codereview.chromium.org/18192/diff/45/217#newcode175 Line 175: std::string::size_type index(0); And remember not to use (0) initialization form when = 0 will work, especially for integral types like this. http://codereview.chromium.org/18192/diff/45/217#newcode240 Line 240: bool WaitForProcessesToExit(const std::wstring& executable_name, Wow, this function really sucks! (Not your code, I know - I'm just saying, the whole interface is terrible.) Instead of copying stuff out of process_util_linux.cc, we should move stuff that we're sharing into process_util_posix.cc. http://codereview.chromium.org/18192/diff/45/217#newcode249 Line 249: base::TimeDelta::FromMilliseconds(wait_milliseconds); Four spaces, not two, on a continuation line. http://codereview.chromium.org/18192/diff/45/216 File net/url_request/url_request_unittest.h (right): http://codereview.chromium.org/18192/diff/45/216#newcode440 Line 440: // When we run on Mac OS X, it fails if there is no wait time. Can you move this into the #if right below, since it applies to the Mac case? This is very interesting to me. We've seen spurious failures in this area, but it never occurred to me that we're not waiting long enough for the server to start up. Theoretically, this wouldn't be a problem just for the Mac. One second is a long time, can we get away with a shorter wait? I bet we can, considering that this rarely fails in most instances without the delay anyway.
http://codereview.chromium.org/18192/diff/45/217 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/45/217#newcode134 Line 134: } One more thing: after you do this second sysctl to get the data, you need to reset the size of the vector to match the number of elements actually returned. You need to do another num_of_kinfo_proc = len / sizeof(struct kinfo_proc); and kinfo_proc_.resize(num_of_kinfo_proc). http://codereview.chromium.org/18192/diff/45/217#newcode155 Line 155: int mib[4] = { KERN_PROCARGS, KERN_PROCARGS, kinfo->kp_proc.p_pid, 0 }; I did a little experimentation here. This one should be: int mib[] = { CTL_KERN, KERN_PROCARGS, kinfo->kp_proc.p_pid }; The difference being that the first element should be CTL_KERN and you only need three elements. http://codereview.chromium.org/18192/diff/45/217#newcode157 Line 157: std::string args; The other thing I found is that this sysctl gives back more than just a string, although the executable name is the first null-terminated run of bytes in what it returns. Also, it's the actual executable path, it's not really argv[0] or any argument, so it's not right to call it args. Let's just call it data. Finally, we should add a comment that says that the sysctl returns more things than we need, but the executable name is in the first run of bytes, so we'll just use that. http://codereview.chromium.org/18192/diff/45/217#newcode167 Line 167: args.resize(args_len - 1); // Don't include the trailing NULL. Since we're just using the string to hold a char buffer, and it may not have a trailing NUL, leave off the -1. http://codereview.chromium.org/18192/diff/45/217#newcode178 Line 178: index = args.find('/', index); I've figured out what you mean here. Instead of finding from the beginning of the string, you should use rfind. But first, you need to find the end of the string (by looking for the first NUL character) - remember, we don't actually want to look at the whole thing returned by sysctl, because it's got way more than just the executable name in it. Something like exec_name_end = data.find('\0'); last_slash = data.rfind('/', exec_name_end);. If last_slash is npos (unlikely), you can take data from 0 to exec_name_end - 1.
On 2009/01/23 15:31:34, Mark Mentovai wrote: > http://codereview.chromium.org/18192/diff/45/217 > File base/process_util_mac.mm (right): > > http://codereview.chromium.org/18192/diff/45/217#newcode134 > Line 134: } > One more thing: after you do this second sysctl to get the data, you need to > reset the size of the vector to match the number of elements actually returned. > You need to do another num_of_kinfo_proc = len / sizeof(struct kinfo_proc); and > kinfo_proc_.resize(num_of_kinfo_proc). > > http://codereview.chromium.org/18192/diff/45/217#newcode155 > Line 155: int mib[4] = { KERN_PROCARGS, KERN_PROCARGS, kinfo->kp_proc.p_pid, 0 > }; > I did a little experimentation here. This one should be: > > int mib[] = { CTL_KERN, KERN_PROCARGS, kinfo->kp_proc.p_pid }; > > The difference being that the first element should be CTL_KERN and you only need > three elements. > > http://codereview.chromium.org/18192/diff/45/217#newcode157 > Line 157: std::string args; > The other thing I found is that this sysctl gives back more than just a string, > although the executable name is the first null-terminated run of bytes in what > it returns. Also, it's the actual executable path, it's not really argv[0] or > any argument, so it's not right to call it args. Let's just call it data. > Finally, we should add a comment that says that the sysctl returns more things > than we need, but the executable name is in the first run of bytes, so we'll > just use that. > > http://codereview.chromium.org/18192/diff/45/217#newcode167 > Line 167: args.resize(args_len - 1); // Don't include the trailing NULL. > Since we're just using the string to hold a char buffer, and it may not have a > trailing NUL, leave off the -1. > > http://codereview.chromium.org/18192/diff/45/217#newcode178 > Line 178: index = args.find('/', index); > I've figured out what you mean here. Instead of finding from the beginning of > the string, you should use rfind. But first, you need to find the end of the > string (by looking for the first NUL character) - remember, we don't actually > want to look at the whole thing returned by sysctl, because it's got way more > than just the executable name in it. > > Something like exec_name_end = data.find('\0'); last_slash = data.rfind('/', > exec_name_end);. If last_slash is npos (unlikely), you can take data from 0 to > exec_name_end - 1.
http://codereview.chromium.org/18192/diff/28/30 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/28/30#newcode50 Line 50: On 2009/01/22 10:21:15, Dean McNamee wrote: > Extra end of line space here. Done. http://codereview.chromium.org/18192/diff/45/218 File base/process_util.h (right): http://codereview.chromium.org/18192/diff/45/218#newcode22 Line 22: #include <vector> On 2009/01/22 15:45:34, Mark Mentovai wrote: > I wouldn't bother conditionalizing this #include, it's completely harmless on > other platforms, and the #if is ugly. Done. http://codereview.chromium.org/18192/diff/45/218#newcode225 Line 225: std::vector<kinfo_proc> kinfo_proc_; On 2009/01/22 15:45:34, Mark Mentovai wrote: > I had suggested renaming this kinfo_procs_ to indicate that it's plural. That's > handy where the definition that says "vector" isn't necessarily visible. Done. http://codereview.chromium.org/18192/diff/45/217 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/45/217#newcode123 Line 123: DLOG(ERROR) << "Failed to call sysctl."; On 2009/01/22 15:45:34, Mark Mentovai wrote: > Nit: we DID call sysctl, but IT failed. Reword the message? Same below. Done. http://codereview.chromium.org/18192/diff/45/217#newcode123 Line 123: DLOG(ERROR) << "Failed to call sysctl."; On 2009/01/22 15:45:34, Mark Mentovai wrote: > Nit: we DID call sysctl, but IT failed. Reword the message? Same below. Done. http://codereview.chromium.org/18192/diff/45/217#newcode123 Line 123: DLOG(ERROR) << "Failed to call sysctl."; On 2009/01/22 15:45:34, Mark Mentovai wrote: > Nit: we DID call sysctl, but IT failed. Reword the message? Same below. Done. http://codereview.chromium.org/18192/diff/45/217#newcode125 Line 125: } else { On 2009/01/22 15:45:34, Mark Mentovai wrote: > No "else" branch needed after a return. Done. http://codereview.chromium.org/18192/diff/45/217#newcode125 Line 125: } else { On 2009/01/22 15:45:34, Mark Mentovai wrote: > No "else" branch needed after a return. Done. http://codereview.chromium.org/18192/diff/45/217#newcode130 Line 130: if (sysctl(mib, arraysize(mib), &kinfo_proc_[0], &len, NULL, 0) == 0) { On 2009/01/22 15:45:34, Mark Mentovai wrote: > You can't pass &len without resetting it to be equivalent to the resized > kinfo_procs_. sysctl needs to know that we're leaving extra room for process > table growth. Before this line, you need: > > len = num_of_kinfo_procs * sizeof(struct kinfo_proc) > > Also, restructure this to get rid of the empty "if" branch. > > Finally, you should decide between whether to treat "== 0" or "any positive > integer" as sysctl success. You've done a couple different things with your > sysctl tests, you should be consistent. Done. http://codereview.chromium.org/18192/diff/45/217#newcode130 Line 130: if (sysctl(mib, arraysize(mib), &kinfo_proc_[0], &len, NULL, 0) == 0) { On 2009/01/22 15:45:34, Mark Mentovai wrote: > You can't pass &len without resetting it to be equivalent to the resized > kinfo_procs_. sysctl needs to know that we're leaving extra room for process > table growth. Before this line, you need: > > len = num_of_kinfo_procs * sizeof(struct kinfo_proc) > > Also, restructure this to get rid of the empty "if" branch. > > Finally, you should decide between whether to treat "== 0" or "any positive > integer" as sysctl success. You've done a couple different things with your > sysctl tests, you should be consistent. Done. http://codereview.chromium.org/18192/diff/45/217#newcode134 Line 134: } On 2009/01/23 15:31:34, Mark Mentovai wrote: > One more thing: after you do this second sysctl to get the data, you need to > reset the size of the vector to match the number of elements actually returned. > You need to do another num_of_kinfo_proc = len / sizeof(struct kinfo_proc); and > kinfo_proc_.resize(num_of_kinfo_proc). Done. http://codereview.chromium.org/18192/diff/45/217#newcode151 Line 151: while(index_of_kinfo_proc_ < kinfo_proc_.size()) { On 2009/01/22 10:21:15, Dean McNamee wrote: > space after while Done. http://codereview.chromium.org/18192/diff/45/217#newcode151 Line 151: while(index_of_kinfo_proc_ < kinfo_proc_.size()) { On 2009/01/22 15:45:34, Mark Mentovai wrote: > This would be better written as: > > for (; index_of_kinfo_proc_ < kinfo_proc_.size(); ++index_of_kinfo_proc_) > > That lets you get rid of the three separate ++index_of_kinfo_proc_ statements in > the loop body, and means that you don't have to remember to add another each > time you add a "continue". Done. http://codereview.chromium.org/18192/diff/45/217#newcode152 Line 152: if(kinfo_proc_[index_of_kinfo_proc_].kp_proc.p_stat != SZOMB) { On 2009/01/22 10:21:15, Dean McNamee wrote: > space after if Done. http://codereview.chromium.org/18192/diff/45/217#newcode155 Line 155: int mib[4] = { KERN_PROCARGS, KERN_PROCARGS, kinfo->kp_proc.p_pid, 0 }; On 2009/01/23 15:31:34, Mark Mentovai wrote: > I did a little experimentation here. This one should be: > > int mib[] = { CTL_KERN, KERN_PROCARGS, kinfo->kp_proc.p_pid }; > > The difference being that the first element should be CTL_KERN and you only need > three elements. Done. http://codereview.chromium.org/18192/diff/45/217#newcode157 Line 157: std::string args; On 2009/01/22 15:45:34, Mark Mentovai wrote: > I would move this declaration down until you actually need it, where you start > using it. Done. http://codereview.chromium.org/18192/diff/45/217#newcode157 Line 157: std::string args; On 2009/01/22 15:45:34, Mark Mentovai wrote: > I would move this declaration down until you actually need it, where you start > using it. Done. http://codereview.chromium.org/18192/diff/45/217#newcode162 Line 162: //DLOG(DEBUG) << "Failed to call sysctrl."; On 2009/01/22 15:45:34, Mark Mentovai wrote: > Why are these commented out? Why does it say sysctrl (emphasis on the r)? Do > we need this or not? Done. http://codereview.chromium.org/18192/diff/45/217#newcode162 Line 162: //DLOG(DEBUG) << "Failed to call sysctrl."; On 2009/01/22 15:45:34, Mark Mentovai wrote: > Why are these commented out? Why does it say sysctrl (emphasis on the r)? Do > we need this or not? Done. http://codereview.chromium.org/18192/diff/45/217#newcode167 Line 167: args.resize(args_len - 1); // Don't include the trailing NULL. On 2009/01/22 15:45:34, Mark Mentovai wrote: > Style nit: two spaces before an inline comment. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem... Done. http://codereview.chromium.org/18192/diff/45/217#newcode174 Line 174: // Get exec name from args with '/'. On 2009/01/22 15:45:34, Mark Mentovai wrote: > Once again, this section is a little bit opaque. It really needs a comment to > explain what's supposed to be going on here. I think that there are bugs, but I > can't tell for sure unless I know what you're trying to do. > > "Get exec name from args with '/'" isn't enough, can you describe how the string > is to be processed? Done. http://codereview.chromium.org/18192/diff/45/217#newcode175 Line 175: std::string::size_type index(0); On 2009/01/22 15:45:34, Mark Mentovai wrote: > And remember not to use (0) initialization form when = 0 will work, especially > for integral types like this. Done. http://codereview.chromium.org/18192/diff/45/217#newcode176 Line 176: while(true) { On 2009/01/22 10:21:15, Dean McNamee wrote: > space after while Done. http://codereview.chromium.org/18192/diff/45/217#newcode179 Line 179: if(index == std::string::npos) { On 2009/01/22 10:21:15, Dean McNamee wrote: > space after if Done. http://codereview.chromium.org/18192/diff/45/217#newcode194 Line 194: if(index_of_kinfo_proc_ >= kinfo_proc_.size()) On 2009/01/22 10:21:15, Dean McNamee wrote: > space Done. http://codereview.chromium.org/18192/diff/45/217#newcode202 Line 202: len = NAME_MAX; On 2009/01/22 10:21:15, Dean McNamee wrote: > use std::min ? Done. http://codereview.chromium.org/18192/diff/45/217#newcode211 Line 211: if (WideToASCII(executable_name_) != entry_.szExeFile) On 2009/01/22 10:21:15, Dean McNamee wrote: > In the case of OSX, this situation might be different. Is the exe file perhaps > in UTF8, and you can use WideToUTF8 and everything will work correctly? Done. http://codereview.chromium.org/18192/diff/45/217#newcode240 Line 240: bool WaitForProcessesToExit(const std::wstring& executable_name, On 2009/01/22 15:45:34, Mark Mentovai wrote: > Wow, this function really sucks! (Not your code, I know - I'm just saying, the > whole interface is terrible.) > > Instead of copying stuff out of process_util_linux.cc, we should move stuff that > we're sharing into process_util_posix.cc. Moved to process_util_posix.cc. http://codereview.chromium.org/18192/diff/45/217#newcode249 Line 249: base::TimeDelta::FromMilliseconds(wait_milliseconds); On 2009/01/22 15:45:34, Mark Mentovai wrote: > Four spaces, not two, on a continuation line. Done. http://codereview.chromium.org/18192/diff/45/216 File net/url_request/url_request_unittest.h (right): http://codereview.chromium.org/18192/diff/45/216#newcode440 Line 440: // When we run on Mac OS X, it fails if there is no wait time. On 2009/01/22 15:45:34, Mark Mentovai wrote: > Can you move this into the #if right below, since it applies to the Mac case? > > This is very interesting to me. We've seen spurious failures in this area, but > it never occurred to me that we're not waiting long enough for the server to > start up. Theoretically, this wouldn't be a problem just for the Mac. > > One second is a long time, can we get away with a shorter wait? I bet we can, > considering that this rarely fails in most instances without the delay anyway. Done.
Could you check again ? On 2009/01/24 07:27:33, honten wrote: > http://codereview.chromium.org/18192/diff/28/30 > File base/process_util_mac.mm (right): > > http://codereview.chromium.org/18192/diff/28/30#newcode50 > Line 50: > On 2009/01/22 10:21:15, Dean McNamee wrote: > > Extra end of line space here. > > Done. > > http://codereview.chromium.org/18192/diff/45/218 > File base/process_util.h (right): > > http://codereview.chromium.org/18192/diff/45/218#newcode22 > Line 22: #include <vector> > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > I wouldn't bother conditionalizing this #include, it's completely harmless on > > other platforms, and the #if is ugly. > > Done. > > http://codereview.chromium.org/18192/diff/45/218#newcode225 > Line 225: std::vector<kinfo_proc> kinfo_proc_; > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > I had suggested renaming this kinfo_procs_ to indicate that it's plural. > That's > > handy where the definition that says "vector" isn't necessarily visible. > > Done. > > http://codereview.chromium.org/18192/diff/45/217 > File base/process_util_mac.mm (right): > > http://codereview.chromium.org/18192/diff/45/217#newcode123 > Line 123: DLOG(ERROR) << "Failed to call sysctl."; > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > Nit: we DID call sysctl, but IT failed. Reword the message? Same below. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode123 > Line 123: DLOG(ERROR) << "Failed to call sysctl."; > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > Nit: we DID call sysctl, but IT failed. Reword the message? Same below. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode123 > Line 123: DLOG(ERROR) << "Failed to call sysctl."; > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > Nit: we DID call sysctl, but IT failed. Reword the message? Same below. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode125 > Line 125: } else { > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > No "else" branch needed after a return. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode125 > Line 125: } else { > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > No "else" branch needed after a return. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode130 > Line 130: if (sysctl(mib, arraysize(mib), &kinfo_proc_[0], &len, NULL, 0) == 0) > { > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > You can't pass &len without resetting it to be equivalent to the resized > > kinfo_procs_. sysctl needs to know that we're leaving extra room for process > > table growth. Before this line, you need: > > > > len = num_of_kinfo_procs * sizeof(struct kinfo_proc) > > > > Also, restructure this to get rid of the empty "if" branch. > > > > Finally, you should decide between whether to treat "== 0" or "any positive > > integer" as sysctl success. You've done a couple different things with your > > sysctl tests, you should be consistent. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode130 > Line 130: if (sysctl(mib, arraysize(mib), &kinfo_proc_[0], &len, NULL, 0) == 0) > { > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > You can't pass &len without resetting it to be equivalent to the resized > > kinfo_procs_. sysctl needs to know that we're leaving extra room for process > > table growth. Before this line, you need: > > > > len = num_of_kinfo_procs * sizeof(struct kinfo_proc) > > > > Also, restructure this to get rid of the empty "if" branch. > > > > Finally, you should decide between whether to treat "== 0" or "any positive > > integer" as sysctl success. You've done a couple different things with your > > sysctl tests, you should be consistent. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode134 > Line 134: } > On 2009/01/23 15:31:34, Mark Mentovai wrote: > > One more thing: after you do this second sysctl to get the data, you need to > > reset the size of the vector to match the number of elements actually > returned. > > You need to do another num_of_kinfo_proc = len / sizeof(struct kinfo_proc); > and > > kinfo_proc_.resize(num_of_kinfo_proc). > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode151 > Line 151: while(index_of_kinfo_proc_ < kinfo_proc_.size()) { > On 2009/01/22 10:21:15, Dean McNamee wrote: > > space after while > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode151 > Line 151: while(index_of_kinfo_proc_ < kinfo_proc_.size()) { > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > This would be better written as: > > > > for (; index_of_kinfo_proc_ < kinfo_proc_.size(); ++index_of_kinfo_proc_) > > > > That lets you get rid of the three separate ++index_of_kinfo_proc_ statements > in > > the loop body, and means that you don't have to remember to add another each > > time you add a "continue". > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode152 > Line 152: if(kinfo_proc_[index_of_kinfo_proc_].kp_proc.p_stat != SZOMB) { > On 2009/01/22 10:21:15, Dean McNamee wrote: > > space after if > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode155 > Line 155: int mib[4] = { KERN_PROCARGS, KERN_PROCARGS, kinfo->kp_proc.p_pid, 0 > }; > On 2009/01/23 15:31:34, Mark Mentovai wrote: > > I did a little experimentation here. This one should be: > > > > int mib[] = { CTL_KERN, KERN_PROCARGS, kinfo->kp_proc.p_pid }; > > > > The difference being that the first element should be CTL_KERN and you only > need > > three elements. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode157 > Line 157: std::string args; > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > I would move this declaration down until you actually need it, where you start > > using it. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode157 > Line 157: std::string args; > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > I would move this declaration down until you actually need it, where you start > > using it. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode162 > Line 162: //DLOG(DEBUG) << "Failed to call sysctrl."; > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > Why are these commented out? Why does it say sysctrl (emphasis on the r)? Do > > we need this or not? > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode162 > Line 162: //DLOG(DEBUG) << "Failed to call sysctrl."; > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > Why are these commented out? Why does it say sysctrl (emphasis on the r)? Do > > we need this or not? > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode167 > Line 167: args.resize(args_len - 1); // Don't include the trailing NULL. > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > Style nit: two spaces before an inline comment. > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem... > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode174 > Line 174: // Get exec name from args with '/'. > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > Once again, this section is a little bit opaque. It really needs a comment to > > explain what's supposed to be going on here. I think that there are bugs, but > I > > can't tell for sure unless I know what you're trying to do. > > > > "Get exec name from args with '/'" isn't enough, can you describe how the > string > > is to be processed? > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode175 > Line 175: std::string::size_type index(0); > On 2009/01/22 15:45:34, Mark Mentovai wrote: > > And remember not to use (0) initialization form when = 0 will work, especially > > for integral types like this. > > Done. > > http://codereview.chromium.org/18192/diff/45/217#newcode176 > Line 176: while(true) { > On 2009/01/22 10:21:15, Dean McNamee wrote: > > space after while > > Done
Another round of comments. Thanks for your patience with this review! http://codereview.chromium.org/18192/diff/234/61 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/234/61#newcode206 Line 206: // Do we have to care about it ? Any reason not to add a NOTIMPLEMENTED() here? If it's not called from anywhere I think we can wait with this for now, I think you can remove the last 2 lines of the TODO. http://codereview.chromium.org/18192/diff/234/64 File base/process_util_posix.cc (right): http://codereview.chromium.org/18192/diff/234/64#newcode294 Line 294: WaitForProcessesToExit(executable_name, wait_milliseconds, Should be indented 4 spaces. http://codereview.chromium.org/18192/diff/234/60 File net/url_request/url_request_unittest.h (right): http://codereview.chromium.org/18192/diff/234/60#newcode442 Line 442: PlatformThread::Sleep(500); Having a sleep here seems kind of flaky, i.e. if one of our builders is loaded and it takes a long time for the process to launch this could fail. Is there another signal we could use to determine that the process has launched that we could wait on in a timeout? Alternately, how about doing a VerifyLaunchApp and then polling again if that fails after a timeout in the case of all platforms?
http://codereview.chromium.org/18192/diff/234/61 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/234/61#newcode206 Line 206: // Do we have to care about it ? On 2009/01/28 18:21:03, jeremy wrote: > Any reason not to add a NOTIMPLEMENTED() here? > > If it's not called from anywhere I think we can wait with this for now, I think > you can remove the last 2 lines of the TODO. Done. http://codereview.chromium.org/18192/diff/234/64 File base/process_util_posix.cc (right): http://codereview.chromium.org/18192/diff/234/64#newcode294 Line 294: WaitForProcessesToExit(executable_name, wait_milliseconds, On 2009/01/28 18:21:03, jeremy wrote: > Should be indented 4 spaces. Done. http://codereview.chromium.org/18192/diff/234/60 File net/url_request/url_request_unittest.h (right): http://codereview.chromium.org/18192/diff/234/60#newcode442 Line 442: PlatformThread::Sleep(500); Ok, This is misunderstood. Even if I removed this part, I looks working fine in my environment. On 2009/01/28 18:21:03, jeremy wrote: > Having a sleep here seems kind of flaky, i.e. if one of our builders is loaded > and it takes a long time for the process to launch this could fail. > > Is there another signal we could use to determine that the process has launched > that we could wait on in a timeout? > > Alternately, how about doing a VerifyLaunchApp and then polling again if that > fails after a timeout in the case of all platforms?
http://codereview.chromium.org/18192/diff/602/403 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/602/403#newcode114 Line 114: Nit: looks like there's some whitespace on this line. http://codereview.chromium.org/18192/diff/602/403#newcode155 Line 155: data.resize(data_len - 1); // Don't include the trailing NULL. Don't do this actually. Remember how we said that data is not just a string, it's a whole blob of stuff. The last byte might not even be a NULL. We shouldn't need the reserve if we just do data.resize(data_len). http://codereview.chromium.org/18192/diff/602/403#newcode169 Line 169: exec_name = data.substr(last_slash + 1).c_str(); We can avoid the c_str call by giving another argument to last_slash: exec_name_end - last_slash - 1. You should do that. http://codereview.chromium.org/18192/diff/602/403#newcode171 Line 171: } And if last_slash is -1, we should just use the whole thing up to exec_name_end as the executable name. exec_name = data.substr(0, exec_name_end). http://codereview.chromium.org/18192/diff/602/403#newcode181 Line 181: const int len = std::min((int)exec_name.size() + 1, NAME_MAX - 1); Looks like you're trying to ensure that szExeFile will have a NUL terminator, but this code is broken. You can use base::strlcpy(entry_.szExeFile, exec_name.c_str(), sizeof(entry_.szExeFile)). No memcpy or std::min necessary.
fyi - this is should also include the remove of the ifdef on KillSlowChild in process_util_unittest
Honten, have you signed and returned a CLA yet? http://code.google.com/legal/individual-cla-v1.0.html
On 2009/02/04 20:31:17, Mark Mentovai wrote: > Honten, have you signed and returned a CLA yet? > > http://code.google.com/legal/individual-cla-v1.0.html I already signed electronically. Is it not enough ?
http://codereview.chromium.org/18192/diff/602/403 File base/process_util_mac.mm (right): http://codereview.chromium.org/18192/diff/602/403#newcode155 Line 155: data.resize(data_len - 1); // Don't include the trailing NULL. On 2009/02/02 17:39:07, Mark Mentovai wrote: > Don't do this actually. Remember how we said that data is not just a string, > it's a whole blob of stuff. The last byte might not even be a NULL. > > We shouldn't need the reserve if we just do data.resize(data_len). Done. http://codereview.chromium.org/18192/diff/602/403#newcode169 Line 169: exec_name = data.substr(last_slash + 1).c_str(); On 2009/02/02 17:39:07, Mark Mentovai wrote: > We can avoid the c_str call by giving another argument to last_slash: > exec_name_end - last_slash - 1. You should do that. Done. http://codereview.chromium.org/18192/diff/602/403#newcode171 Line 171: } On 2009/02/02 17:39:07, Mark Mentovai wrote: > And if last_slash is -1, we should just use the whole thing up to exec_name_end > as the executable name. exec_name = data.substr(0, exec_name_end). Done. http://codereview.chromium.org/18192/diff/602/403#newcode181 Line 181: const int len = std::min((int)exec_name.size() + 1, NAME_MAX - 1); On 2009/02/02 17:39:07, Mark Mentovai wrote: > Looks like you're trying to ensure that szExeFile will have a NUL terminator, > but this code is broken. You can use base::strlcpy(entry_.szExeFile, > exec_name.c_str(), sizeof(entry_.szExeFile)). No memcpy or std::min necessary. Done.
On 2009/02/04 18:13:06, TVL wrote: > fyi - this is should also include the remove of the ifdef on KillSlowChild in > process_util_unittest Ok, I removed ifdef and passed the test.
Could you comment again ? On 2009/02/05 07:01:24, honten wrote: > On 2009/02/04 18:13:06, TVL wrote: > > fyi - this is should also include the remove of the ifdef on KillSlowChild in > > process_util_unittest > > Ok, I removed ifdef and passed the test.
On 2009/02/19 06:27:14, honten wrote: > Could you comment again ? > Actually, isn't this closed? Mark - didn't you land this a while ago? TVL > On 2009/02/05 07:01:24, honten wrote: > > On 2009/02/04 18:13:06, TVL wrote: > > > fyi - this is should also include the remove of the ifdef on KillSlowChild > in > > > process_util_unittest > > > > Ok, I removed ifdef and passed the test.
Ok, I checked the latest source code and somebody checked in... But I want to have any response if it's closed ;-) Anyway, thank you for your many comments and supports. Thanks, On 2009/02/19 15:00:29, TVL wrote: > On 2009/02/19 06:27:14, honten wrote: > > Could you comment again ? > > > > Actually, isn't this closed? > > Mark - didn't you land this a while ago? > > TVL > > > On 2009/02/05 07:01:24, honten wrote: > > > On 2009/02/04 18:13:06, TVL wrote: > > > > fyi - this is should also include the remove of the ifdef on > KillSlowChild > > in > > > > process_util_unittest > > > > > > Ok, I removed ifdef and passed the test.
Mark, Sorry, I looked over your e-mail while I was in vacation. Anyway, thanks a lot!! If I can help other tasks, please tell me. On 2009/02/21 06:11:57, honten wrote: > Ok, > > I checked the latest source code and somebody checked in... > But I want to have any response if it's closed ;-) > > Anyway, thank you for your many comments and supports. > > Thanks, > > On 2009/02/19 15:00:29, TVL wrote: > > On 2009/02/19 06:27:14, honten wrote: > > > Could you comment again ? > > > > > > > Actually, isn't this closed? > > > > Mark - didn't you land this a while ago? > > > > TVL > > > > > On 2009/02/05 07:01:24, honten wrote: > > > > On 2009/02/04 18:13:06, TVL wrote: > > > > > fyi - this is should also include the remove of the ifdef on > > KillSlowChild > > > in > > > > > process_util_unittest > > > > > > > > Ok, I removed ifdef and passed the test. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
