Adding support for generating minidumps to a given file descriptor.

Adding support for generating minidumps to a given file descriptor.

Adding a way to create an ExceptionHandler that takes in a file descriptor
where the minidump should be created, without the need of opening any other
file.

BUG=None
TEST=Run unit-tests.

[Mark Mentovai, 2012-07-28 15:26:21]

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
File client/linux/handler/exception_handler.cc (right):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.cc:196: }  else {
Too many spaces before the else?

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.cc:197: minidump_fd_duplicate_ = 0;
0 is a valid FD value. You should use -1 if you need an invalid value.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.cc:272: // When using a file descriptor,
we don't deal with names.
Avoid saying “we” in comments. Who are “we?” This is clearer if the statement is
more direct, like “When using a file descriptor, don't deal with names.”

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.cc:455: // The generation of the minidump
closes the FD. Make sure we provide a
we -> to

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
File client/linux/handler/exception_handler.h (right):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:94: // file is
<dump_path>\<minidump_id>.dmp.  context is the parameter supplied
This comment seems to be in need of revision.

What’s the point of minidump_id when the descriptor is an FD?

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:139: MinidumpCallback callback,
Just for purposes of comparing the various constructors, you should generally
follow the same format in each case. Here, you’ve put filter and callback on
separate lines, but in the constructor above and below, they’re on the same
line. Personally, I prefer them on different lines, but what matters most is
that they’re consistent.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:152: const int server_fd);
Do you think it would make sense to roll all three of these overloaded
constructors into one constructor that handles all cases? I do.

For example, you can roll the two that have a different first argument
(dump_path/minidump_fd) into a single one that takes a MinidumpDescriptor. And
you can have only one constructor that takes a server_fd parameter and document
that if server_fd is -1, in-process generation will always be used.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:241: // This is needed as generating
the minidump closes the FD, and we might
we

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:242: // still want to write to the
minidump extra-information.
want to write extra information to the minidump

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:243: int minidump_fd_duplicate_;
This field seems cheesey and it’s sort of hard to follow what it’s all about,
when it’ll be set, why it’ll be set, etc., just based on reading the header. I
sort of get it, but would it be possible to make whatever writes the dump (maybe
optionally?) not close the FD to avoid needing this? I don’t like wasting FDs.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
File client/linux/handler/exception_handler_unittest.cc (right):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:63: sprintf(file_name,
"breakpad%d.tmp", random);
Never use sprintf, always use snprintf.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:64: *path = dir;
Is there any assurance that dir ends with a slash?

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:66: return
open(path->c_str(), O_CREAT | O_RDWR);
Rather than doing all of this, I think you should use mkstemp.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:125: close(fds[0]);  // Close
the reading end, we'll write.
we, and again on line 138.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:128: (void*) fds[1], true);
Don’t use (c_style)casts, use cplusplus<style>(casts). Again on line 133.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:135:
*reinterpret_cast<volatile int*>(NULL) = 0;
I think that if you refactor the constructors as I’ve suggested, you can avoid
some of this duplication easily. If not, then you can avoid the duplication like
this:
  {
    scoped_ptr<ExceptionHandler> handler;
    if (use_fd) {
      handler.reset(new ExceptionHandler(…));
    } else {
      handler.reset(new ExceptionHandler(…));
    }
    // Now crash with the exception handler in scope.
    *reinterpret_cast<volatile int*>(NULL) = 0;
  }

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/minidump_d...
File client/linux/handler/minidump_descriptor.h (left):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/minidump_d...
client/linux/handler/minidump_descriptor.h:30: // Inline implementation of
ScopedTaskSuspend, which suspends a Mach
This file looks like a copy, but it’s not a copy. Did you use svn to make a copy
of an unrelated file, or are you using git and did it gittishly assume this new
file is a copy of something it isn’t a copy of?

If the former, don’t do that.

If the latter, I hear this works: add this to your .gitconfig and reuploaded:

[diff]
  renamelimit=1

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/minidump_d...
File client/linux/handler/minidump_descriptor.h (right):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/minidump_d...
client/linux/handler/minidump_descriptor.h:49: explicit MinidumpDescriptor(const
int fd) : path_(NULL), fd_(fd) {
The const here isn’t very helpful. Same on line 56.

It’s equivalent to writing “const char* const path” in the constructor on line
45, but you didn’t do that.

[jcivelli, 2012-08-02 21:41:34]

Per our chat, ExceptionHandler now takes a MinidumpDescriptor that itself takes
either a dir path or a FD.
The descriptor also contains the logic that build the path where the minidump is
generated, and is passed to the callback. That way we can do without the crash
ID and reconstruction of the path.

I did a bit of clean-up in the unit-tests, hope I did not go overboard.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
File client/linux/handler/exception_handler.cc (right):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.cc:196: }  else {
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> Too many spaces before the else?

Done.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.cc:197: minidump_fd_duplicate_ = 0;
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> 0 is a valid FD value. You should use -1 if you need an invalid value.
Oops, right. Done.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.cc:272: // When using a file descriptor,
we don't deal with names.
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> Avoid saying “we” in comments. Who are “we?” This is clearer if the statement
is
> more direct, like “When using a file descriptor, don't deal with names.”
"We", you know, like me. And the guys. :-)
I agree, changed to a more direct statement.
(I haven't changed the other comments with "we" already in that file though -
many of them actually).

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.cc:455: // The generation of the minidump
closes the FD. Make sure we provide a
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> we -> to

Done.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
File client/linux/handler/exception_handler.h (right):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:94: // file is
<dump_path>\<minidump_id>.dmp.  context is the parameter supplied
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> This comment seems to be in need of revision.
> 
> What’s the point of minidump_id when the descriptor is an FD?
Actually, since the MinidumpDescriptor contains the full file path when it is a
file, do we really need the minidump_id?
I removed it, let me know if this is craziness.
The way it works now: you create a MinidumpDescriptor with either a FD of a path
to a dir. If it's a path, the ExceptionHandler creates the full path to the
minidump file when created.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:139: MinidumpCallback callback,
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> Just for purposes of comparing the various constructors, you should generally
> follow the same format in each case. Here, you’ve put filter and callback on
> separate lines, but in the constructor above and below, they’re on the same
> line. Personally, I prefer them on different lines, but what matters most is
> that they’re consistent.

Done.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:152: const int server_fd);
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> Do you think it would make sense to roll all three of these overloaded
> constructors into one constructor that handles all cases? I do.
> 
> For example, you can roll the two that have a different first argument
> (dump_path/minidump_fd) into a single one that takes a MinidumpDescriptor. And
> you can have only one constructor that takes a server_fd parameter and
document
> that if server_fd is -1, in-process generation will always be used.

I agree it does make sense.
Done.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:241: // This is needed as generating
the minidump closes the FD, and we might
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> we
This was removed.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:242: // still want to write to the
minidump extra-information.
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> want to write extra information to the minidump
This was removed.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler.h:243: int minidump_fd_duplicate_;
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> This field seems cheesey and it’s sort of hard to follow what it’s all about,
> when it’ll be set, why it’ll be set, etc., just based on reading the header. I
> sort of get it, but would it be possible to make whatever writes the dump
(maybe
> optionally?) not close the FD to avoid needing this? I don’t like wasting FDs.
Good point about wasting FDs. I changed so we don't close the FD when a FD has
been provided.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
File client/linux/handler/exception_handler_unittest.cc (right):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:63: sprintf(file_name,
"breakpad%d.tmp", random);
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> Never use sprintf, always use snprintf.

Done.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:64: *path = dir;
 2012/07/28 15:26:21, Mark Mentovai wrote:
> Is there any assurance that dir ends with a slash?
Fixed.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:66: return
open(path->c_str(), O_CREAT | O_RDWR);
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> Rather than doing all of this, I think you should use mkstemp.
But... I really want a path, not a FD.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:125: close(fds[0]);  // Close
the reading end, we'll write.
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> we, and again on line 138.

Done.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:128: (void*) fds[1], true);
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> Don’t use (c_style)casts, use cplusplus<style>(casts). Again on line 133.
This was in the original code. I took the opportunity to fix several other
C-casts I found.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/exception_...
client/linux/handler/exception_handler_unittest.cc:135:
*reinterpret_cast<volatile int*>(NULL) = 0;
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> I think that if you refactor the constructors as I’ve suggested, you can avoid
> some of this duplication easily. If not, then you can avoid the duplication
like
> this:
>   {
>     scoped_ptr<ExceptionHandler> handler;
>     if (use_fd) {
>       handler.reset(new ExceptionHandler(…));
>     } else {
>       handler.reset(new ExceptionHandler(…));
>     }
>     // Now crash with the exception handler in scope.
>     *reinterpret_cast<volatile int*>(NULL) = 0;
>   }

Done.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/minidump_d...
File client/linux/handler/minidump_descriptor.h (left):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/minidump_d...
client/linux/handler/minidump_descriptor.h:30: // Inline implementation of
ScopedTaskSuspend, which suspends a Mach
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> This file looks like a copy, but it’s not a copy. Did you use svn to make a
copy
> of an unrelated file, or are you using git and did it gittishly assume this
new
> file is a copy of something it isn’t a copy of?
> 
> If the former, don’t do that.
> 
> If the latter, I hear this works: add this to your .gitconfig and reuploaded:
> 
> [diff]
>   renamelimit=1

It's git being overzealous.
I tried your suggestion, we'll see what happens on my next upload.

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/minidump_d...
File client/linux/handler/minidump_descriptor.h (right):

https://codereview.appspot.com/6450061/diff/1/client/linux/handler/minidump_d...
client/linux/handler/minidump_descriptor.h:49: explicit MinidumpDescriptor(const
int fd) : path_(NULL), fd_(fd) {
On 2012/07/28 15:26:21, Mark Mentovai wrote:
> The const here isn’t very helpful. Same on line 56.
> 
> It’s equivalent to writing “const char* const path” in the constructor on line
> 45, but you didn’t do that.

Done.

[Mark Mentovai, 2012-08-03 21:32:08]

This mostly looks good. Few more comments.

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
File client/linux/handler/exception_handler.cc (left):

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler.cc:493: UpdateNextID();
By getting rid of this UpdateNextID here, do you change anything? I think this
used to be able to generate multiple dumps in sequence, each at a different ID.
What happens now if a caller tries that? If an FD was passed in, does it write a
new dump to the same FD? If a directory was passed in, do you get a new unique
dump path, or does the new dump overwrite the old?

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
File client/linux/handler/exception_handler.cc (right):

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler.cc:139: crash_handler_ = NULL;
Why not set this up in the initializer list above: crash_handler_(NULL)?

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler.cc:140: if (0 <= server_fd)
|server_fd >= 0| reads more naturally.

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler.cc:144: InstallHandlers();
This looks funny because it seems to say “if the handler is installed, install
the handler.” I can see that handler_installed_ is just set to the value of the
install_handler argument, but it’s not clear unless you look at that assignment.
I would make this condition check install_handler instead of handler_installed_,
because that reads more rationally.

In fact, it appears that there’s no longer any reason to track
handler_installed_ in a member variable. It’s not used anywhere, is it?

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
File client/linux/handler/exception_handler_unittest.cc (right):

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler_unittest.cc:66: snprintf(file_name, 128,
"breakpad%d.tmp", random);
Use arraysize(file_name) (or sizeof(file_name)) instead of hard-coding 128
twice.

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler_unittest.cc:70: return
open(path->c_str(), O_CREAT | O_RDWR);
You can use mkstemp[s] to get a path: call it, and it will have transformed the
template argument to be the path it used to create the file. You also get an FD
as a return value, but if you don’t want to use the FD, you can just close it.
This really is a more secure and better way to create temporary files.

[jcivelli, 2012-08-06 18:16:32]

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
File client/linux/handler/exception_handler.cc (left):

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler.cc:493: UpdateNextID();
On 2012/08/03 21:32:08, Mark Mentovai wrote:
> By getting rid of this UpdateNextID here, do you change anything? I think this
> used to be able to generate multiple dumps in sequence, each at a different
ID.
> What happens now if a caller tries that? If an FD was passed in, does it write
a
> new dump to the same FD? If a directory was passed in, do you get a new unique
> dump path, or does the new dump overwrite the old?
Good point.
I made sure we still support generating minidumps in sequence. With path we'll
generate a new path before generating the minidump (before so the
MinidumpDescriptor retrieved from the ExceptionHandler has the right path - and
you don't need to use a callback to retrieve it).
With a FD, we reposition the FD to its beginning before generating a new
minidump.
I added unit-tests to test that.

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
File client/linux/handler/exception_handler.cc (right):

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler.cc:139: crash_handler_ = NULL;
On 2012/08/03 21:32:08, Mark Mentovai wrote:
> Why not set this up in the initializer list above: crash_handler_(NULL)?

Done.

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler.cc:140: if (0 <= server_fd)
On 2012/08/03 21:32:08, Mark Mentovai wrote:
> |server_fd >= 0| reads more naturally.

Done.

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler.cc:144: InstallHandlers();
On 2012/08/03 21:32:08, Mark Mentovai wrote:
> This looks funny because it seems to say “if the handler is installed, install
> the handler.”&nbsp;I can see that handler_installed_ is just set to the value
of the
> install_handler argument, but it’s not clear unless you look at that
assignment.
> I would make this condition check install_handler instead of
handler_installed_,
> because that reads more rationally.
> 
> In fact, it appears that there’s no longer any reason to track
> handler_installed_ in a member variable. It’s not used anywhere, is it?

Good point, handler_installed_ is not used anymore.
I removed it.

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
File client/linux/handler/exception_handler_unittest.cc (right):

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler_unittest.cc:66: snprintf(file_name, 128,
"breakpad%d.tmp", random);
On 2012/08/03 21:32:08, Mark Mentovai wrote:
> Use arraysize(file_name) (or sizeof(file_name)) instead of hard-coding 128
> twice.

This code is gone.

https://codereview.appspot.com/6450061/diff/9012/client/linux/handler/excepti...
client/linux/handler/exception_handler_unittest.cc:70: return
open(path->c_str(), O_CREAT | O_RDWR);
On 2012/08/03 21:32:08, Mark Mentovai wrote:
> You can use mkstemp[s] to get a path: call it, and it will have transformed
the
> template argument to be the path it used to create the file. You also get an
FD
> as a return value, but if you don’t want to use the FD, you can just close it.
> This really is a more secure and better way to create temporary files.
Actually I do need the FD, so using mkstemp makes a lot of sense.
Done.

[Mark Mentovai, 2012-08-06 20:30:26]

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
File client/linux/handler/exception_handler.cc (right):

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler.cc:455: // Reposition the FD to its
beginning.
You’ll be overwriting, but what if the new dump is shorter than the old? Do you
want to truncate the file with ftruncate when you do this?

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
File client/linux/handler/exception_handler_unittest.cc (right):

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler_unittest.cc:64: std::string file = dir +
"/exception-handler-unittest-XXXXXX";
Tiny nit: it’s usual to separate the Xs from the rest of the name with a dot
instead of a dash.

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler_unittest.cc:65: const char* c_file =
file.c_str();
You’ve written more code than you need. Let’s go through some options.

Option A: instead of making a copy of the string and having to free it, why not
juse &file[0] to get a non-const char* that you’re allowed to write into? (Yup,
really, you are.)

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler_unittest.cc:69: strncpy(c_path, c_file,
len);
Option B: instead of strnlen-new-strncpy, why not just call strdup? (You’ll need
to use free instead of delete[] if you do this).

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler_unittest.cc:72: *path = c_path;
You can avoid this copy too by using *path in all of your operations instead of
file.

[jcivelli, 2012-08-06 21:01:03]

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
File client/linux/handler/exception_handler.cc (right):

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler.cc:455: // Reposition the FD to its
beginning.
On 2012/08/06 20:30:26, Mark Mentovai wrote:
> You’ll be overwriting, but what if the new dump is shorter than the old? Do
you
> want to truncate the file with ftruncate when you do this?
Very good point. Now truncating.

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
File client/linux/handler/exception_handler_unittest.cc (right):

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler_unittest.cc:64: std::string file = dir +
"/exception-handler-unittest-XXXXXX";
On 2012/08/06 20:30:26, Mark Mentovai wrote:
> Tiny nit: it’s usual to separate the Xs from the rest of the name with a dot
> instead of a dash.

Done.

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler_unittest.cc:65: const char* c_file =
file.c_str();
On 2012/08/06 20:30:26, Mark Mentovai wrote:
> You’ve written more code than you need. Let’s go through some options.
> 
> Option A: instead of making a copy of the string and having to free it, why
not
> juse &file[0] to get a non-const char* that you’re allowed to write into?
(Yup,
> really, you are.)
I did not know it's OK to do that. There seems to be a bit of controversy on
that from what I found searching about it. (potential issue if the string
internal data is not stored in a contiguous array). So I'll go with option B to
be extra safe.

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler_unittest.cc:69: strncpy(c_path, c_file,
len);
On 2012/08/06 20:30:26, Mark Mentovai wrote:
> Option B: instead of strnlen-new-strncpy, why not just call strdup? (You’ll
need
> to use free instead of delete[] if you do this).

Done.

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler_unittest.cc:72: *path = c_path;
On 2012/08/06 20:30:26, Mark Mentovai wrote:
> You can avoid this copy too by using *path in all of your operations instead
of
> file.
I understand this could only work with option A.

[Mark Mentovai, 2012-08-06 21:05:32]

(Re-review to follow in the coming minutes, just responding to your comment.)

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
File client/linux/handler/exception_handler_unittest.cc (right):

https://codereview.appspot.com/6450061/diff/13001/client/linux/handler/except...
client/linux/handler/exception_handler_unittest.cc:65: const char* c_file =
file.c_str();
jcivelli wrote:
> On 2012/08/06 20:30:26, Mark Mentovai wrote:
> > You’ve written more code than you need. Let’s go through some options.
> > 
> > Option A: instead of making a copy of the string and having to free it, why
> not
> > juse &file[0] to get a non-const char* that you’re allowed to write into?
> (Yup,
> > really, you are.)
> I did not know it's OK to do that. There seems to be a bit of controversy on
> that from what I found searching about it. (potential issue if the string
> internal data is not stored in a contiguous array). So I'll go with option B
to
> be extra safe.

This shouldn’t be that controversial. See WriteInto in Chromium’s
base/string_util.h. The behavior we rely on here is guaranteed in C++11, and
although it isn’t guaranteed pre-C++11, all C++ standard library implementations
we support have been compatible with our use. Since C++11 does now guarantee
this behavior, it’s exceedingly unlikely that any future library or future
version of any current library we care about will break this.

[Mark Mentovai, 2012-08-06 22:01:28]

LGTM