Initial version of crossystem.

Initial version of crossystem.  

Works for getting switch positions, hwid, fwid.

BUG=chrome-os-partner:1940
TEST=ran manually on Mario and Alex

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=5421866

[vb, 2011-02-05 01:17:18]

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c
File host/lib/crossystem.c (right):

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c#newcode15
host/lib/crossystem.c:15: // GPIO signal types
A nit: do we use C++ comments in S sources?

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c#newcode21
host/lib/crossystem.c:21: #define CHSW_RECOVERY_BOOT     0x00000002
haven't these flags been already defined somewhere else in the vboot space?

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c#newcode49
host/lib/crossystem.c:49: got = fgets(dest, size, f);
what if fgets() fails?

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c#newcode68
host/lib/crossystem.c:68: if (!*buf || e == buf)
what's !*buf checking?

[Randall Spangler, 2011-02-07 17:58:10]

Updated code.  PTAL.

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c
File host/lib/crossystem.c (right):

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c#newcode15
host/lib/crossystem.c:15: // GPIO signal types
On 2011/02/05 01:17:18, vb wrote:
> A nit: do we use C++ comments in S sources?

Done.

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c#newcode21
host/lib/crossystem.c:21: #define CHSW_RECOVERY_BOOT     0x00000002
On 2011/02/05 01:17:18, vb wrote:
> haven't these flags been already defined somewhere else in the vboot space?

No, interestingly enough.  CHSW is currently provided via ACPI from H2C, and
vboot uses a different set of flags to LoadKernel() and LoadFirmware().

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c#newcode49
host/lib/crossystem.c:49: got = fgets(dest, size, f);
On 2011/02/05 01:17:18, vb wrote:
> what if fgets() fails?
got() returns NULL, which is passed through by ReadFileString() to mean error.

http://codereview.chromium.org/6413002/diff/1/host/lib/crossystem.c#newcode68
host/lib/crossystem.c:68: if (!*buf || e == buf)
On 2011/02/05 01:17:18, vb wrote:
> what's !*buf checking?

Empty string, but in that case, ReadFileString() would have failed.  Removed.

[Bill Richardson, 2011-02-07 18:49:51]

One error, a couple of nits.

http://codereview.chromium.org/6413002/diff/4001/host/include/crossystem.h
File host/include/crossystem.h (right):

http://codereview.chromium.org/6413002/diff/4001/host/include/crossystem.h#ne...
host/include/crossystem.h:15: * size.
Might also comment that the returned string will be truncated and
null-terminated if the dest is too small.

http://codereview.chromium.org/6413002/diff/4001/host/lib/crossystem.c
File host/lib/crossystem.c (right):

http://codereview.chromium.org/6413002/diff/4001/host/lib/crossystem.c#newcode68
host/lib/crossystem.c:68: if (e == buf)
No, you had it right before. If the first arg to strtol() is null, you get a
segfault. If *buf is '\0', you get a zero as the return value. The only way for
strtol() to return an error is if you check explicitly, either before or after
the call. Checking after is better because you can see if there were any
extraneous characters left after the conversion.

http://codereview.chromium.org/6413002/diff/4001/host/lib/crossystem.c#newcod...
host/lib/crossystem.c:120: if (strcmp(controller_name, "NM10"))
I like explict 0 != strcmp(), but I can't find any mention of it in the C++
style guides. Up to you.

http://codereview.chromium.org/6413002/diff/4001/host/lib/crossystem.c#newcod...
host/lib/crossystem.c:150: return (value == active_high ? 1 : 0);
return value == active_high;

[Bill Richardson, 2011-02-07 19:04:42]

Sorry, LGTM.

http://codereview.chromium.org/6413002/diff/4001/host/lib/crossystem.c
File host/lib/crossystem.c (right):

http://codereview.chromium.org/6413002/diff/4001/host/lib/crossystem.c#newcode68
host/lib/crossystem.c:68: if (e == buf)
No, wait. You're doing something different here. Nevermind.