Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(324)

Issue 9794002: [microdump] Add build fingerprint and product info metadata.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 9 months ago by primiano
Modified:
2 years, 9 months ago
CC:
google-breakpad-dev_googlegroups.com
Base URL:
http://google-breakpad.googlecode.com/svn/trunk
Visibility:
Public.

Description

[microdump] Add build fingerprint and product info metadata.

This is to add build fingerprint and product name/version to
microdumps. Conversely to what happens in the case of minidumps
with MIME fields, due to the nature of minidumps, extra metadata
cannot be reliably injected after the dump is completed.
This CL adds the plumbing to inject two optional fields plus the
corresponding tests.

BUG=chromium:410294

Patch Set 1 #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M src/client/linux/handler/exception_handler.cc View 1 chunk +7 lines, -4 lines 0 comments Download
M src/client/linux/handler/minidump_descriptor.cc View 3 chunks +16 lines, -1 line 0 comments Download
M src/client/linux/handler/minidump_descriptor.h View 4 chunks +34 lines, -4 lines 3 comments Download
M src/client/linux/microdump_writer/microdump_writer.cc View 8 chunks +41 lines, -15 lines 0 comments Download
M src/client/linux/microdump_writer/microdump_writer.h View 1 chunk +5 lines, -1 line 2 comments Download
M src/client/linux/microdump_writer/microdump_writer_unittest.cc View 4 chunks +57 lines, -27 lines 3 comments Download

Messages

Total messages: 4
primiano
Mark/Lei can I ask a review on this? This cl is adding two strings to ...
2 years, 9 months ago #1
Lei Zhang (chromium)
https://breakpad.appspot.com/9794002/diff/1/src/client/linux/handler/minidump_descriptor.h File src/client/linux/handler/minidump_descriptor.h (right): https://breakpad.appspot.com/9794002/diff/1/src/client/linux/handler/minidump_descriptor.h#newcode110 src/client/linux/handler/minidump_descriptor.h:110: void SetMicrodumpBuildFingerprint(const char* build_fingerprint); Do you see the need ...
2 years, 9 months ago #2
primiano
https://breakpad.appspot.com/9794002/diff/1/src/client/linux/handler/minidump_descriptor.h File src/client/linux/handler/minidump_descriptor.h (right): https://breakpad.appspot.com/9794002/diff/1/src/client/linux/handler/minidump_descriptor.h#newcode110 src/client/linux/handler/minidump_descriptor.h:110: void SetMicrodumpBuildFingerprint(const char* build_fingerprint); On 2015/03/19 19:42:58, Lei Zhang ...
2 years, 9 months ago #3
Lei Zhang (chromium)
2 years, 9 months ago #4
https://breakpad.appspot.com/9794002/diff/1/src/client/linux/handler/minidump...
File src/client/linux/handler/minidump_descriptor.h (right):

https://breakpad.appspot.com/9794002/diff/1/src/client/linux/handler/minidump...
src/client/linux/handler/minidump_descriptor.h:110: void
SetMicrodumpBuildFingerprint(const char* build_fingerprint);
On 2015/03/19 20:14:39, primiano wrote:
> On 2015/03/19 19:42:58, Lei Zhang (chromium) wrote:
> > Do you see the need to do this separately from the ctor, or can the two set
> > methods be omitted if you just change the MicrodumpOnConsole version of the
> > ctor?
> > 
> > That would require a single CL to do the Breakpad roll + change on the
> Chromium
> > side, unless you temporarly have 2 MicrodumpOnConsole ctors.
> 
> Yeah the only reason for this was to make eventual reverts easy / not leave
> breakpad in an unrollable state.
> I'm happy with putting them in the ctor if that is you prefer.

Sure. We can certainly change this later once things stabilize.

https://breakpad.appspot.com/9794002/diff/1/src/client/linux/microdump_writer...
File src/client/linux/microdump_writer/microdump_writer_unittest.cc (right):

https://breakpad.appspot.com/9794002/diff/1/src/client/linux/microdump_writer...
src/client/linux/microdump_writer/microdump_writer_unittest.cc:57: EXPECT_NE(-1,
pipe(fds));
On 2015/03/19 20:14:39, primiano wrote:
> On 2015/03/19 19:42:58, Lei Zhang (chromium) wrote:
> > Shouldn't we bail out with ASSERT_FOO like before on failures?
> 
> The problem is that once you are in a void helper function ASSERT generates
code
> that doesn't compile anymore (since it tries to return some gtest magic).
> I don't know honestly a better alternative to keep the ASSERTS as such in a
> helper fn. Or I can rewrite them as:
> if (pipe() == -1)  { FAIL() << "pipe()"; return false}
> 
> and ASSERT_TRUE the call in the test fixture.

It compiles fine here with ASSERTs with make
./src/client/linux/linux_client_unittest
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1004:630ec63f810e-tainted