Fill in more fields of struct MDRawSystemInfo.

Fill in more fields of struct MDRawSystemInfo.

BUG=none
TEST=none

This is code from Breakpad's /client/linux/handler/minidump_generator.cc.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=18986

[agl, 2009-06-22 21:37:26]

http://codereview.chromium.org/144002/diff/1/3
File breakpad/linux/minidump_writer.cc (right):

http://codereview.chromium.org/144002/diff/1/3#newcode329
Line 329: static const unsigned kNumWriters = 10;
Don't remove cpuinfo from the dump

http://codereview.chromium.org/144002/diff/1/3#newcode621
Line 621: const char* proc_cpu_path = "/proc/cpuinfo";
static const char proc_cpu_path[]

http://codereview.chromium.org/144002/diff/1/3#newcode623
Line 623: char vendor_id[13];
Look like this should be a sizeof() based on line 692

http://codereview.chromium.org/144002/diff/1/3#newcode624
Line 624: const char vendor_id_name[] = "vendor_id";
static

http://codereview.chromium.org/144002/diff/1/3#newcode625
Line 625: const size_t vendor_id_name_length = sizeof(vendor_id_name) - 1;
static

http://codereview.chromium.org/144002/diff/1/3#newcode645
Line 645: if (!strncmp(line, entry->info_name, strlen(entry->info_name))) {
This will match any prefix. There are lines for "model" and "model name" for
example.

http://codereview.chromium.org/144002/diff/1/3#newcode648
Line 648: if (value)
If the return value of strchr was NULL, then you just incremented it and made it
true.

http://codereview.chromium.org/144002/diff/1/3#newcode649
Line 649: sscanf(value, " %d", &(entry->value));
There's only one space after the :, so didn't the value++ consume it?

http://codereview.chromium.org/144002/diff/1/3#newcode651
Line 651: entry++;
This while loop should be a for loop.

http://codereview.chromium.org/144002/diff/1/3#newcode655
Line 655: if (!strncmp(line, vendor_id_name, vendor_id_name_length)) {
prefix problem again.

http://codereview.chromium.org/144002/diff/1/3#newcode661
Line 661: while (*value && isspace(*value))
The first half of the conjunction is superfluous since isspace(0) == false

http://codereview.chromium.org/144002/diff/1/3#newcode666
Line 666: if (value[length - 1] == '\n')
probably want to check for length == 0

[Lei Zhang, 2009-06-22 23:20:46]

http://codereview.chromium.org/144002/diff/1/3
File breakpad/linux/minidump_writer.cc (right):

http://codereview.chromium.org/144002/diff/1/3#newcode329
Line 329: static const unsigned kNumWriters = 10;
On 2009/06/22 21:37:26, agl wrote:
> Don't remove cpuinfo from the dump

Ok, reverted.

http://codereview.chromium.org/144002/diff/1/3#newcode621
Line 621: const char* proc_cpu_path = "/proc/cpuinfo";
On 2009/06/22 21:37:26, agl wrote:
> static const char proc_cpu_path[]

added all the statics.

http://codereview.chromium.org/144002/diff/1/3#newcode623
Line 623: char vendor_id[13];
On 2009/06/22 21:37:26, agl wrote:
> Look like this should be a sizeof() based on line 692

Done, the 13 value is sizeof() + 1.

http://codereview.chromium.org/144002/diff/1/3#newcode645
Line 645: if (!strncmp(line, entry->info_name, strlen(entry->info_name))) {
On 2009/06/22 21:37:26, agl wrote:
> This will match any prefix. There are lines for "model" and "model name" for
> example.

I'm checking for nothing but spaces between the end of the prefix and the colon.
Can you think of something better?

http://codereview.chromium.org/144002/diff/1/3#newcode648
Line 648: if (value)
On 2009/06/22 21:37:26, agl wrote:
> If the return value of strchr was NULL, then you just incremented it and made
it
> true.

Fixed. (sigh, I need to throw out the notion that this is all reviewed, sane
code.)

http://codereview.chromium.org/144002/diff/1/3#newcode649
Line 649: sscanf(value, " %d", &(entry->value));
On 2009/06/22 21:37:26, agl wrote:
> There's only one space after the :, so didn't the value++ consume it?

no, value++ consumed the ':'.

http://codereview.chromium.org/144002/diff/1/3#newcode651
Line 651: entry++;
On 2009/06/22 21:37:26, agl wrote:
> This while loop should be a for loop.

Done.

http://codereview.chromium.org/144002/diff/1/3#newcode661
Line 661: while (*value && isspace(*value))
On 2009/06/22 21:37:26, agl wrote:
> The first half of the conjunction is superfluous since isspace(0) == false

first half removed.

http://codereview.chromium.org/144002/diff/1/3#newcode666
Line 666: if (value[length - 1] == '\n')
On 2009/06/22 21:37:26, agl wrote:
> probably want to check for length == 0

done.

[agl, 2009-06-22 23:26:27]

LGTM

http://codereview.chromium.org/144002/diff/1006/1007
File breakpad/linux/minidump_writer.cc (right):

http://codereview.chromium.org/144002/diff/1006/1007#newcode646
Line 646: MD_CPU_ARCHITECTURE_AMD64;
#else
#error "Unknown CPU arch"

<- probably a good idea

[Lei Zhang, 2009-06-22 23:29:20]

On 2009/06/22 23:26:27, agl wrote:
> LGTM
> 
> http://codereview.chromium.org/144002/diff/1006/1007
> File breakpad/linux/minidump_writer.cc (right):
> 
> http://codereview.chromium.org/144002/diff/1006/1007#newcode646
> Line 646: MD_CPU_ARCHITECTURE_AMD64;
> #else
> #error "Unknown CPU arch"
> 
> <- probably a good idea

Indeed. Done, tested, and committed.