Introduce microdump writer class.

Introduce microdump writer class.

Microdumps are a very lightweight variant of minidumps. They are meant
to dump a minimal crash report on the system log (logcat on Android),
containing only the state of the crashing thread.
This is to deal with cases where the user has opted out from crash
uploading but we still want to generate meaningful information on the
device to pull a stacktrace for development purposes.
Conversely to conventional stack traces (e.g. the one generated by
Android's debuggerd or Chromium's base::stacktrace) microdumps do NOT
require unwind tables to be present in the target binary. This allows
to save precious binary size (~1.5 MB for Chrome on Arm, ~10 MB on
arm64).
More information and design doc on crbug.com/410294

BUG=chromium:410294

[primiano]

There we go... finally getting to the juicy content.
Maria would you mind start taking a look to this? It still need some small
finishings from my side, but that is mostly it.

Note: this won't have any effect in Chrome right now. A corresponding change is
required there and will come soon.

[mmandlis]

I haven't looked in details into the microdump_writer code yet, but I'm sending
you my comments so far.

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/exceptio...
File src/client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/exceptio...
src/client/linux/handler/exception_handler.cc:200: 
nit - is the extra blank line intentional?

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/exceptio...
src/client/linux/handler/exception_handler.cc:596:
!minidump_descriptor_.IsMicrodump()) {
Is it ok to call this method when IsMicrodump() is true? Maybe you should add a
CHECK (or, less aggressively, if+log(warn)+return)?

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
File src/client/linux/handler/minidump_descriptor.cc (right):

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
src/client/linux/handler/minidump_descriptor.cc:50:
assert(descriptor.path_.empty());
is it valid to have both - is_microdump_ set to true, and directory_ non-empty?
maybe we should add some validation checks here? or anywhere at all?

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
File src/client/linux/handler/minidump_descriptor.h (right):

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
src/client/linux/handler/minidump_descriptor.h:40: // The MinidumpDescriptor
describes how to access a minidump: it can contain
please, update the comment.

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
src/client/linux/handler/minidump_descriptor.h:50: MinidumpDescriptor() :
is_microdump_(true), fd_(-1), size_limit_(-1) {}
You're providing a whole new meaning to the MinidumpDescriptor's empty ctor
here.
I notice that it is never used, but still doesn't seem healthy practice to me. I
think if the caller is about to create a what is actually MicrodumpDescriptor,
it should be done explicitly.

It is sad enough that this class is called Minidump... (I'm not asking you to
change this, because I think this will create too much noise in the codebase and
is not worth the effort)..

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

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/microdump_writer...
src/client/linux/microdump_writer/microdump_writer.cc:83: LogCommitLine();
There is a call to LogCommitLine() already in the DumpOSinformation, is this
intentional?

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/microdump_writer...
File src/client/linux/microdump_writer/microdump_writer.h (right):

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/microdump_writer...
src/client/linux/microdump_writer/microdump_writer.h:3: 
please, add comment (I know, you said you're not done yet, so you were probably
planning to add it anyway...)

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/microdump_writer...
src/client/linux/microdump_writer/microdump_writer.h:7: #include
"client/linux/minidump_writer/minidump_writer.h"
I see that you need this include for the MappingList - is there any common place
where it could be moved, so that this is cleaner?

Alternatively, since the distinction between micro and mini dump
writers/descriptors is not completely clean, maybe it's simpler to just  declare
WriteMicrodump(..) in minidump_writer.h altogether?

[primiano]

Thanks for the review. It should look in a better state now.

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/exceptio...
File src/client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/exceptio...
src/client/linux/handler/exception_handler.cc:200: 
On 2014/10/15 23:45:36, mmandlis wrote:
> nit - is the extra blank line intentional?

Hmm I guess not :)

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/exceptio...
src/client/linux/handler/exception_handler.cc:596:
!minidump_descriptor_.IsMicrodump()) {
On 2014/10/15 23:45:36, mmandlis wrote:
> Is it ok to call this method when IsMicrodump() is true? Maybe you should add
a
> CHECK (or, less aggressively, if+log(warn)+return)?

WriteMinidump() can be called by various call sites (in Chrome) to actually
trigger the dump, e.g. in the case of a spontaneous crash / CHECK (so, yes, it
can be called from here).

The sense of this check is to avoid the call to UpdatePath (which has already
the proper guards inside, to check that the minidump refers to a valid dir).

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
File src/client/linux/handler/minidump_descriptor.cc (right):

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
src/client/linux/handler/minidump_descriptor.cc:50:
assert(descriptor.path_.empty());
On 2014/10/15 23:45:36, mmandlis wrote:
> is it valid to have both - is_microdump_ set to true, and directory_
non-empty?
> maybe we should add some validation checks here? or anywhere at all?

No, but I don't think neither the copy constructor nor the assignment one should
check it. that field is private and at current state nobody (except
getMicrodumpDescriptor() ) can set it.

I added all the validation in exception_handler (hopefully I didn't miss
anything), where the struct is actually used

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
File src/client/linux/handler/minidump_descriptor.h (right):

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
src/client/linux/handler/minidump_descriptor.h:40: // The MinidumpDescriptor
describes how to access a minidump: it can contain
On 2014/10/15 23:45:36, mmandlis wrote:
> please, update the comment.

Done.

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/handler/minidump...
src/client/linux/handler/minidump_descriptor.h:50: MinidumpDescriptor() :
is_microdump_(true), fd_(-1), size_limit_(-1) {}
On 2014/10/15 23:45:36, mmandlis wrote:
> You're providing a whole new meaning to the MinidumpDescriptor's empty ctor
> here.
> I notice that it is never used, but still doesn't seem healthy practice to me.
I
> think if the caller is about to create a what is actually MicrodumpDescriptor,
> it should be done explicitly.
> 
> It is sad enough that this class is called Minidump... (I'm not asking you to
> change this, because I think this will create too much noise in the codebase
and
> is not worth the effort)..
>  

Right I changed this file a bit.
Reset the default ctor to false and introduced a getMicrodumpDescriptor()
factory method instead of using kMicrodump. Should look better now, I think.
(Yes, the fact that we have to still use minidump_descriptor is unfortunate. On
the other side, refactoring this class + the chrome side just for a bool doesn't
seem really worth to me).

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

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/microdump_writer...
src/client/linux/microdump_writer/microdump_writer.cc:83: LogCommitLine();
On 2014/10/15 23:45:36, mmandlis wrote:
> There is a call to LogCommitLine() already in the DumpOSinformation, is this
> intentional?

Oh good catch. No that is just wrong :)

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/microdump_writer...
File src/client/linux/microdump_writer/microdump_writer.h (right):

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/microdump_writer...
src/client/linux/microdump_writer/microdump_writer.h:3: 
On 2014/10/15 23:45:36, mmandlis wrote:
> please, add comment (I know, you said you're not done yet, so you were
probably
> planning to add it anyway...) 

Done.

https://breakpad.appspot.com/5754002/diff/1/src/client/linux/microdump_writer...
src/client/linux/microdump_writer/microdump_writer.h:7: #include
"client/linux/minidump_writer/minidump_writer.h"
On 2014/10/15 23:45:36, mmandlis wrote:
> I see that you need this include for the MappingList - is there any common
place
> where it could be moved, so that this is cleaner?
> 
> Alternatively, since the distinction between micro and mini dump
> writers/descriptors is not completely clean, maybe it's simpler to just 
declare
> WriteMicrodump(..) in minidump_writer.h altogether?

Hmm I think I solved this in a different way, take a look to the new patchset.

[mmandlis]

looks good overall. couple more nits, but mostly questions to help me better
internalize this code.

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/dump_writer...
File src/client/linux/dump_writer_common/mapping_info.h (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/dump_writer...
src/client/linux/dump_writer_common/mapping_info.h:36: #include <string.h>
is string really used?

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
File src/client/linux/handler/minidump_descriptor.cc (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
src/client/linux/handler/minidump_descriptor.cc:42: return md;
Do we not care about the object being copied when returned? Is it not better to
return a pointer?

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
File src/client/linux/handler/minidump_descriptor.h (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
src/client/linux/handler/minidump_descriptor.h:41: // either a file descriptor,
a path or indicate to generate only a microdump
nit - s/indicate/indicator?

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
src/client/linux/handler/minidump_descriptor.h:72: MinidumpDescriptor(const
MinidumpDescriptor& descriptor);
did you intentionally remove the 'explicit' here?

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
src/client/linux/handler/minidump_descriptor.h:100: bool
microdump_on_console_only_;
optional - I'd drop the 'only' part of the name (here and the Should... method.
It is making the name a bit on the longer side, and it is not really meaningful.

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
File src/client/linux/microdump_writer/microdump_writer.cc (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.cc:313: !strstr(mapping.name,
".so")) {
All the mappings in the proof-of-concept microdump were .so, why are you not
including them now?
Or were these the "mappings provided by the caller" that handled below?

Do you mind explaining in 2 words the difference between these two? thanks.

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.cc:320: if
(!DumpModule(mapping, true, i, NULL))
nit - looking at the code, DumpModule actually always returns true, so there
isn't much use in the return value.
However theoretically assuming it could fail, would you not want to just skip
the particular module and try to proceed?

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.cc:326: ++iter) {
you don't check the mapping size here, like you do above, why the difference?

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
File src/client/linux/microdump_writer/microdump_writer.h (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.h:3: 
Shouldn't there be a standard "copyright, redistribution, ..." comment in this
file? You did add one in the .cc

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.h:16: //   crashing_process:
the pid of the crashing process. This must be trusted.
please, excuse my ignorance, what does 'trusted' mean here?

[primiano]

Thanks. See replies inline.

Mark: can I ask you a comment about minidump_descriptor.h ?
Does the overall interface change make sense to you?
What about the name of the new method: Maria was suggesting to remove the "Only"
part from the name. Comments? Other ideas?

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/dump_writer...
File src/client/linux/dump_writer_common/mapping_info.h (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/dump_writer...
src/client/linux/dump_writer_common/mapping_info.h:36: #include <string.h>
On 2014/10/17 21:20:17, mmandlis wrote:
> is string really used?

Oh, right, this might be due to some code I put here and took back.
Good catch.

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
File src/client/linux/handler/minidump_descriptor.cc (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
src/client/linux/handler/minidump_descriptor.cc:42: return md;
On 2014/10/17 21:20:17, mmandlis wrote:
> Do we not care about the object being copied when returned? Is it not better
to
> return a pointer?
The main problem is: a pointer to what? If I want to return a pointer I need to
create a static global somewhere, but then I have to introduce (yet more)
boilerplate to initialize microdump_on_console_only_  to true.
Furthermore, returning a pointer is IMHO a bit odd: that means start thinking
about who owns this and how to pass the pointer around (does breakpad even have
smart pointers?).

This struct is a descriptor with 3 POD data types, and in the current
implementation it is already passed by copy everywhere else (which is very
reasonable, considering that we are talking about moving 12 bytes vs 4 on 32 bit
and 12 vs 8 on 64 bit) :)

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
File src/client/linux/handler/minidump_descriptor.h (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
src/client/linux/handler/minidump_descriptor.h:41: // either a file descriptor,
a path or indicate to generate only a microdump
On 2014/10/17 21:20:17, mmandlis wrote:
> nit - s/indicate/indicator?
I reworded it a bit. Should look better now.

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
src/client/linux/handler/minidump_descriptor.h:72: MinidumpDescriptor(const
MinidumpDescriptor& descriptor);
On 2014/10/17 21:20:17, mmandlis wrote:
> did you intentionally remove the 'explicit' here?

Yes to be able to use the copy constructor implicitly on return (the other
places where the descriptor is currently  copied use the assignment copy
constructor, e.g. descriptor_ = ...). I think nothing is currently using this
copy constructor so far (besides the change I'm introducing).

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/handler/min...
src/client/linux/handler/minidump_descriptor.h:100: bool
microdump_on_console_only_;
On 2014/10/17 21:20:17, mmandlis wrote:
> optional - I'd drop the 'only' part of the name (here and the Should...
method.
> It is making the name a bit on the longer side, and it is not really
meaningful.
Hmm the plan was the emphasize that this is not generating /uploading any
microdump, but only logging on the console.
Naming this ShouldWriteMicrodumpOnConsole (-only), IMHO leaves unclear to who
reads the code if this is still uploading a minidump or not (it's not).
Let's ask Mark if he has other proposals?

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
File src/client/linux/microdump_writer/microdump_writer.cc (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.cc:313: !strstr(mapping.name,
".so")) {
On 2014/10/17 21:20:17, mmandlis wrote:
> All the mappings in the proof-of-concept microdump were .so, why are you not
> including them now?
Oh, it's the other way round, I'm including *only* .so files and skipping all
the boring stuff (fonts, apks, etc) :)
I did reword this (also this check subsumes the != '\0', hence I'm removing
that) check.
> Or were these the "mappings provided by the caller" that handled below?
 
> Do you mind explaining in 2 words the difference between these two? thanks.
Uh sure: the maps provided by the caller (mapping_lsit_) are extra-mapping that
the original binary can provide. Essentially this is telling breakpad "whenever
you will dump mappings, would you mind appending these extra ones? Trust me they
are useful". These can provide extra magic information which might not be
directly available when looking into /proc/maps.
Concrete example: Mozilla introduced this to inform breakpad that they are
mmap-ing a .so from the app's .apk (breakpad would be oblivious about the latter
mapping, because it has no way to know the name of the .so, and would only
perceive the .apk mapping here).
We, in Chrome, are in the process of doing something similar. At current state,
we are using a slightly different mechanism (chrome folks have *taught* breakpad
how to extract the file name). However, since this feature (mmap .so from .apk)
is still WiP, I felt leaving the extra lines about mapping_list_ because they
might be useful if they change slightly their plans.

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.cc:320: if
(!DumpModule(mapping, true, i, NULL))
On 2014/10/17 21:20:17, mmandlis wrote:
> nit - looking at the code, DumpModule actually always returns true, so there
> isn't much use in the return value.
Oh, good point. Removing the checks. There is no point having unused
boilerplate.

> However theoretically assuming it could fail, would you not want to just skip
> the particular module and try to proceed?

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.cc:326: ++iter) {
On 2014/10/17 21:20:17, mmandlis wrote:
> you don't check the mapping size here, like you do above, why the difference?
In principle, if the app adds some extra mappings, they should be meaningful and
useful, and there isn't much point checking if we should skip them. The
principle here is that the app knows something more than breakpad if it decides
to push extra mappings.
(plus, IIRC, I copied this behavior from minidump_writer.cc :) )

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
File src/client/linux/microdump_writer/microdump_writer.h (right):

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.h:3: 
On 2014/10/17 21:20:17, mmandlis wrote:
> Shouldn't there be a standard "copyright, redistribution, ..." comment in this
> file? You did add one in the .cc

Oh, actually, yes!

https://breakpad.appspot.com/5754002/diff/110005/src/client/linux/microdump_w...
src/client/linux/microdump_writer/microdump_writer.h:16: //   crashing_process:
the pid of the crashing process. This must be trusted.
On 2014/10/17 21:20:17, mmandlis wrote:
> please, excuse my ignorance, what does 'trusted' mean here?

To be honest, I copied this from minidump_writer :)
*I think* that the deal is that this piece of code has no way to determine
whether that pid is a valid number or is the result of corrupted memory. It has
to trust the fact that is valid, and if not will very likely crash the crash
handler together with the crashed process :)

[Mark Mentovai]

https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/mini...
File src/client/linux/handler/minidump_descriptor.h (right):

https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/mini...
src/client/linux/handler/minidump_descriptor.h:41: // either a file descriptor,
a path or a bool indicating to generate only a
Logically, it’s either an FD, a path, or a flag that says to write a microdump
to the console. It doesn’t “contain” a bool in the same way that it might
contain an FD or a path, because there’s no way for the bool to be false.

https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/mini...
src/client/linux/handler/minidump_descriptor.h:70: 
I think you should have a constructor to form the on-console-only variant.
Otherwise, it’s not clear whether the on-console microdump is in addition to or
instead of the minidump.

You can do this with a constructor that accepts an argument of a custom type.

class MinidumpDescriptor {
 public:
  struct MicrodumpOnConsole {};
  const MicrodumpOnConsole kMicrodumpOnConsole;
  // … other constructors…
  explicit MicrodumpDescriptor(const MicrodumpOnConsole&)
      : microdump_on_console_only_(true),
        // …
};

https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/mini...
src/client/linux/handler/minidump_descriptor.h:100: bool
microdump_on_console_only_;
Again here, the way that the name is structured, it says “the microdump should
be written on the console only” but leaves open the question about whether a
minidump should be written. “only_write_microdump_on_console_” would be clearer.

If you’ve made it clearer based on the interface (as suggested above) that the
class can only write a minidump to a path, a minidump to an FD, or a microdump
to the console, you can get rid of “only” altogether.

A better option would be to make the operating mode much more explicit:

  enum WhatAndWhereToWrite {
    kWriteMinidumpToFD;
    kWriteMinidumpToPath;
    kWriteMicrodumpToConsole;
  };

[mmandlis]

I'm happy with all the other changes, and +1 to Mark's comments.

[primiano]

Many thanks Mark for the comments. Does it look better now?

https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/mini...
File src/client/linux/handler/minidump_descriptor.h (right):

https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/mini...
src/client/linux/handler/minidump_descriptor.h:41: // either a file descriptor,
a path or a bool indicating to generate only a
On 2014/10/21 16:01:45, Mark Mentovai wrote:
> Logically, it’s either an FD, a path, or a flag that says to write a microdump
> to the console. It doesn’t “contain” a bool in the same way that it might
> contain an FD or a path, because there’s no way for the bool to be false.
Yeah, that was what I was striving to constraint, but I admit that it looked a
bit odd.
I like the pattern you suggested, bought it ;)

https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/mini...
src/client/linux/handler/minidump_descriptor.h:70: 
On 2014/10/21 16:01:45, Mark Mentovai wrote:
> I think you should have a constructor to form the on-console-only variant.
> Otherwise, it’s not clear whether the on-console microdump is in addition to
or
> instead of the minidump.
> 
> You can do this with a constructor that accepts an argument of a custom type.
> 
> class MinidumpDescriptor {
>  public:
>   struct MicrodumpOnConsole {};
>   const MicrodumpOnConsole kMicrodumpOnConsole;
>   // … other constructors…
>   explicit MicrodumpDescriptor(const MicrodumpOnConsole&)
>       : microdump_on_console_only_(true),
>         // …
> };

Done.

https://breakpad.appspot.com/5754002/diff/40002/src/client/linux/handler/mini...
src/client/linux/handler/minidump_descriptor.h:100: bool
microdump_on_console_only_;
On 2014/10/21 16:01:45, Mark Mentovai wrote:
> Again here, the way that the name is structured, it says “the microdump should
> be written on the console only” but leaves open the question about whether a
> minidump should be written. “only_write_microdump_on_console_” would be
clearer.
> 
> If you’ve made it clearer based on the interface (as suggested above) that the
> class can only write a minidump to a path, a minidump to an FD, or a microdump
> to the console, you can get rid of “only” altogether.

Yeah agree.

 
> A better option would be to make the operating mode much more explicit:
> 
>   enum WhatAndWhereToWrite {
>     kWriteMinidumpToFD;
>     kWriteMinidumpToPath;
>     kWriteMicrodumpToConsole;
>   };

Hmm but that would change significantly the API interface, and than all the
other clients would have to slightly change their code to roll newer versions of
breakpad. To be honest I agree that it would be cleaner but I don't feel there
is a strong need that justifies breaking the API. With the changes you proposed,
the interface should be fool proof anyways.

[Mark Mentovai]

primiano wrote:
> > A better option would be to make the operating mode much more explicit:
> > 
> >   enum WhatAndWhereToWrite {
> >     kWriteMinidumpToFD;
> >     kWriteMinidumpToPath;
> >     kWriteMicrodumpToConsole;
> >   };
> 
> Hmm but that would change significantly the API interface, and than all the
> other clients would have to slightly change their code to roll newer versions
of
> breakpad. To be honest I agree that it would be cleaner but I don't feel there
> is a strong need that justifies breaking the API. With the changes you
proposed,
> the interface should be fool proof anyways.

I meant to keep the distinct constructors for putting an object into a certain
“when-and-where” mode, so that the external interface remains the same (but with
the new “microdump-console” constructor addition). The enum would be private,
and there’d be a private member variable, so that an object’s mode would be much
clearer than “if this member is false, if that member is not -1, if this other
member is not empty…”

[primiano]

Oh, I completely misunderstood the suggestion.
Makes sense. Done!

[Mark Mentovai]

That looks like the way to go now!

I only looked at minidump_descriptor.h.

[mmandlis]

lgtm

Btw, are you going to add some tests for the microdump_writer class? I'm ok with
doing it in a later cl, but we really should have some coverage.

Thanks.

[primiano]

> Btw, are you going to add some tests for the microdump_writer class? I'm ok
with
> doing it in a later cl, but we really should have some coverage.
Yes, I promise :)
The main reason why the test are not here is: in order to test this class I need
a the microdump processor to be there to close the loop with end-2-end testing
(like in minidump_writer_unittest.cc).
But in order to get the processor I assume we need this :)

Thanks, will land soon

[mmandlis]

Tests - are the best way to check whether a person can keep his promise)))

As soon as you send me an example of generated microdump with its symbols, I'll
be able to adjust my cl and send it to you for review.

thanks!

On 2014/10/23 10:43:36, primiano wrote:
> > Btw, are you going to add some tests for the microdump_writer class? I'm ok
> with
> > doing it in a later cl, but we really should have some coverage.
> Yes, I promise :)
> The main reason why the test are not here is: in order to test this class I
need
> a the microdump processor to be there to close the loop with end-2-end testing
> (like in minidump_writer_unittest.cc).
> But in order to get the processor I assume we need this :)
> 
> Thanks, will land soon

[primiano]

Committed as r1398