first cut: establishes base helper classes and main

first cut: establishes base helper classes and main

cros_boot_mode supports determining the boot mode of a Chrome OS device.
By default, unsupported devices will be considered to be in "normal"
mode and CrOS devices with custom platform support will export the
required interfaces to support indicating "developer" mode.

In addition, there is support for short-circuiting the detection
using a kernel commandline parameter to enable arbitrary testing
of a single, untouched image on supported hardware without recompiling
the kernel, flipping switch, etc.

NOTE: common.mk was copied over from verity/common.mk.
Moved here from http://codereview.chromium.org/3432027/show


TEST=unittests
BUG=chromium-os:824, chromium-os:6864

[jrbarnette, 2010-09-29 19:25:05]

Some minor comments to start with; I may have more to say in a bit.

http://codereview.chromium.org/3530001/diff/13001/14008
File boot_mode_main.cc (right):

http://codereview.chromium.org/3530001/diff/13001/14008#newcode23
boot_mode_main.cc:23: "  -unsupported_as_developer\n"
shouldn't the help show this with two '-' characters?  As in
  --unsupported_as_developer

That seems to be what the man page for getopt_long_only() says
is the behavior.

http://codereview.chromium.org/3530001/diff/13001/14008#newcode25
boot_mode_main.cc:25: "  -ignore_bootloader\n"
Same comment regarding '--'.

http://codereview.chromium.org/3530001/diff/13001/14008#newcode27
boot_mode_main.cc:27: "  -strict_match\n"
Same comment regarding '--'

http://codereview.chromium.org/3530001/diff/13001/14008#newcode29
boot_mode_main.cc:29: "  -in_mode\n"
Same comment regarding '--'

[Will Drewry, 2010-09-29 21:45:52]

On 2010/09/29 19:25:05, jrbarnette wrote:
> Some minor comments to start with; I may have more to say in a bit.
> 
> http://codereview.chromium.org/3530001/diff/13001/14008
> File boot_mode_main.cc (right):
> 
> http://codereview.chromium.org/3530001/diff/13001/14008#newcode23
> boot_mode_main.cc:23: "  -unsupported_as_developer\n"
> shouldn't the help show this with two '-' characters?  As in
>   --unsupported_as_developer
> 
> That seems to be what the man page for getopt_long_only() says
> is the behavior.


Nah - long_only specifically supports - or --. From my local man page:
""
getopt_long_only() is like getopt_long(), but '-' as well as '--' can indicate a
long option. If an option that starts with '-' (not '--') doesn't match a long
option, but does match a short option, it is parsed as a short option instead.
""

I do this to keep consistency with the way chrome's libbase command line options
look/work.

> http://codereview.chromium.org/3530001/diff/13001/14008#newcode25
> boot_mode_main.cc:25: "  -ignore_bootloader\n"
> Same comment regarding '--'.
> 
> http://codereview.chromium.org/3530001/diff/13001/14008#newcode27
> boot_mode_main.cc:27: "  -strict_match\n"
> Same comment regarding '--'
> 
> http://codereview.chromium.org/3530001/diff/13001/14008#newcode29
> boot_mode_main.cc:29: "  -in_mode\n"
> Same comment regarding '--'

[Will Drewry, 2010-09-29 21:47:42]

[+sosa so he can see all the pieces]

On 2010/09/29 21:45:52, Will Drewry wrote:
> On 2010/09/29 19:25:05, jrbarnette wrote:
> > Some minor comments to start with; I may have more to say in a bit.
> > 
> > http://codereview.chromium.org/3530001/diff/13001/14008
> > File boot_mode_main.cc (right):
> > 
> > http://codereview.chromium.org/3530001/diff/13001/14008#newcode23
> > boot_mode_main.cc:23: "  -unsupported_as_developer\n"
> > shouldn't the help show this with two '-' characters?  As in
> >   --unsupported_as_developer
> > 
> > That seems to be what the man page for getopt_long_only() says
> > is the behavior.
> 
> 
> Nah - long_only specifically supports - or --. From my local man page:
> ""
> getopt_long_only() is like getopt_long(), but '-' as well as '--' can indicate
a
> long option. If an option that starts with '-' (not '--') doesn't match a long
> option, but does match a short option, it is parsed as a short option instead.
> ""
> 
> I do this to keep consistency with the way chrome's libbase command line
options
> look/work.
> 
> > http://codereview.chromium.org/3530001/diff/13001/14008#newcode25
> > boot_mode_main.cc:25: "  -ignore_bootloader\n"
> > Same comment regarding '--'.
> > 
> > http://codereview.chromium.org/3530001/diff/13001/14008#newcode27
> > boot_mode_main.cc:27: "  -strict_match\n"
> > Same comment regarding '--'
> > 
> > http://codereview.chromium.org/3530001/diff/13001/14008#newcode29
> > boot_mode_main.cc:29: "  -in_mode\n"
> > Same comment regarding '--'

[Will Drewry, 2010-09-30 19:35:58]

Any takers for a final review /lgtm? I was really hoping to land this set before
the end of the week :/

thanks!

[jrbarnette, 2010-09-30 20:30:17]

I have some (unimportant) typo notes.

More substantively, I have two questions:
  * Am I correct in reading that "class BootReason" is unused and untested?
  * Am I correct in reading that "class RecoveryButton" is unused except for
   its unit tests?

http://codereview.chromium.org/3530001/diff/13001/14002
File Makefile (right):

http://codereview.chromium.org/3530001/diff/13001/14002#newcode23
Makefile:23: # Tests are not build by default.
typo: "built"

http://codereview.chromium.org/3530001/diff/13001/14015
File common.mk (right):

http://codereview.chromium.org/3530001/diff/13001/14015#newcode7
common.mk:7: # can be built in the source tree or with the build products places
elsewhere.
typo: "placed"

http://codereview.chromium.org/3530001/diff/13001/14015#newcode16
common.mk:16: #    $(BUILD)relative/path/target: ...
Is this correct?  Makefile uses "$(OUT)path/target:", and there's no BUILD
defined in this file (but OUT is).

[Will Drewry, 2010-09-30 20:40:36]

On Thu, Sep 30, 2010 at 3:30 PM,  <jrbarnette@chromium.org> wrote:
> I have some (unimportant) typo notes.
>
> More substantively, I have two questions:
>  * Am I correct in reading that "class BootReason" is unused and untested?
>  * Am I correct in reading that "class RecoveryButton" is unused except for
>   its unit tests?

Yup. I started down the path with BootReason and RecoveryButton at
Randall's recommendation, but once I started to explore the firmware
specification and the platform interfaces, I realized that neither of
those values are important now for use in cros_boot_mode.  However, I
didn't see any reason not to keep the code around for extended use --
even if not in cros_boot_mode itself.

I can add a basic unittest on bootreason and let it lie, or if you
prefer drop both from the CL.  (I'm not picky :)

I'll reply to the specific comments inline as I fix them.

Thanks!
will

>
>
>
> http://codereview.chromium.org/3530001/diff/13001/14002
> File Makefile (right):
>
> http://codereview.chromium.org/3530001/diff/13001/14002#newcode23
> Makefile:23: # Tests are not build by default.
> typo: "built"
>
> http://codereview.chromium.org/3530001/diff/13001/14015
> File common.mk (right):
>
> http://codereview.chromium.org/3530001/diff/13001/14015#newcode7
> common.mk:7: # can be built in the source tree or with the build
> products places elsewhere.
> typo: "placed"
>
> http://codereview.chromium.org/3530001/diff/13001/14015#newcode16
> common.mk:16: #    $(BUILD)relative/path/target: ...
> Is this correct?  Makefile uses "$(OUT)path/target:", and there's no
> BUILD
> defined in this file (but OUT is).
>
> http://codereview.chromium.org/3530001/show
>

[Will Drewry, 2010-09-30 20:43:35]

Nice catches - thanks!

http://codereview.chromium.org/3530001/diff/13001/14002
File Makefile (right):

http://codereview.chromium.org/3530001/diff/13001/14002#newcode23
Makefile:23: # Tests are not build by default.
On 2010/09/30 20:30:17, jrbarnette wrote:
> typo: "built"
> 

Done.

http://codereview.chromium.org/3530001/diff/13001/14015
File common.mk (right):

http://codereview.chromium.org/3530001/diff/13001/14015#newcode7
common.mk:7: # can be built in the source tree or with the build products places
elsewhere.
On 2010/09/30 20:30:17, jrbarnette wrote:
> typo: "placed"
> 

Done.

http://codereview.chromium.org/3530001/diff/13001/14015#newcode16
common.mk:16: #    $(BUILD)relative/path/target: ...
On 2010/09/30 20:30:17, jrbarnette wrote:
> Is this correct?  Makefile uses "$(OUT)path/target:", and there's no BUILD
> defined in this file (but OUT is).
> 

Done.

[sosa, 2010-09-30 20:46:00]

http://codereview.chromium.org/3530001/diff/13001/14002
File Makefile (right):

http://codereview.chromium.org/3530001/diff/13001/14002#newcode23
Makefile:23: # Tests are not build by default.
s/build/built

http://codereview.chromium.org/3530001/diff/13001/14011
File boot_reason.cc (right):

http://codereview.chromium.org/3530001/diff/13001/14011#newcode38
boot_reason.cc:38: return kUnsupported;
You may want to consider checking /root/.dev_mode for unsupported ... i.e. if
exists return developer otherwise normal.  This is the case for unsupported
firmware so we can continue to test this with those devices.

[Will Drewry, 2010-09-30 20:51:26]

Thanks!

What do you think about an ImageType class? Maybe it could be enabled (or
disabled) as premptory using something like -i / -use_image_type_first or
something similar.  I do worry about how much many paths we'll have, but I'm
cool centralizing it all first :)

http://codereview.chromium.org/3530001/diff/13001/14002
File Makefile (right):

http://codereview.chromium.org/3530001/diff/13001/14002#newcode23
Makefile:23: # Tests are not build by default.
On 2010/09/30 20:46:00, sosa wrote:
> s/build/built

Done.

http://codereview.chromium.org/3530001/diff/13001/14011
File boot_reason.cc (right):

http://codereview.chromium.org/3530001/diff/13001/14011#newcode38
boot_reason.cc:38: return kUnsupported;
On 2010/09/30 20:46:00, sosa wrote:
> You may want to consider checking /root/.dev_mode for unsupported ... i.e. if
> exists return developer otherwise normal.  This is the case for unsupported
> firmware so we can continue to test this with those devices.

I don't want to pollute the logic in this class with hidden filesystem secrets. 
This file specifically instruments the Boot Reason code from the firmware.  The
BootMode front end class provides the centralization of this sort of logic.

We can certainly add that in -- if you look at how the bootloader override is
done, it is similar.  We can add a 

ImageType class which will check .dev_mode, .test?, etc and could be used to
pre-empt further detection or replace unsupported with "developer".  That said,
there is some conflation between image and mode and I'm not sure where it makes
sense to draw the line.

For your query, we can test on unsupported devices because they will still
operate in "normal" mode, but if a password is stamped, it will be usable (eg on
a test image).  However, we can add a ImageType into the mix and start to remove
it from being hardcoded everywhere else!

[sosa, 2010-09-30 21:03:17]

That SGTM.  Right now our image class equivalent is all over the place
and I would like to move abstract all this logic into one place so we
can easily change the implementation underneath calling code.

On Thu, Sep 30, 2010 at 1:51 PM,  <wad@chromium.org> wrote:
> Thanks!
>
> What do you think about an ImageType class? Maybe it could be enabled (or
> disabled) as premptory using something like -i / -use_image_type_first or
> something similar.  I do worry about how much many paths we'll have, but I'm
> cool centralizing it all first :)
>
>
> http://codereview.chromium.org/3530001/diff/13001/14002
> File Makefile (right):
>
> http://codereview.chromium.org/3530001/diff/13001/14002#newcode23
> Makefile:23: # Tests are not build by default.
> On 2010/09/30 20:46:00, sosa wrote:
>>
>> s/build/built
>
> Done.
>
> http://codereview.chromium.org/3530001/diff/13001/14011
> File boot_reason.cc (right):
>
> http://codereview.chromium.org/3530001/diff/13001/14011#newcode38
> boot_reason.cc:38: return kUnsupported;
> On 2010/09/30 20:46:00, sosa wrote:
>>
>> You may want to consider checking /root/.dev_mode for unsupported ...
>
> i.e. if
>>
>> exists return developer otherwise normal.  This is the case for
>
> unsupported
>>
>> firmware so we can continue to test this with those devices.
>
> I don't want to pollute the logic in this class with hidden filesystem
> secrets.  This file specifically instruments the Boot Reason code from
> the firmware.  The BootMode front end class provides the centralization
> of this sort of logic.
>
> We can certainly add that in -- if you look at how the bootloader
> override is done, it is similar.  We can add a
>
> ImageType class which will check .dev_mode, .test?, etc and could be
> used to pre-empt further detection or replace unsupported with
> "developer".  That said, there is some conflation between image and mode
> and I'm not sure where it makes sense to draw the line.
>
> For your query, we can test on unsupported devices because they will
> still operate in "normal" mode, but if a password is stamped, it will be
> usable (eg on a test image).  However, we can add a ImageType into the
> mix and start to remove it from being hardcoded everywhere else!
>
> http://codereview.chromium.org/3530001/show
>

[jrbarnette, 2010-09-30 21:14:56]

On Sep 30, 2010, at 1:40 PM, Will Drewry wrote:

> On Thu, Sep 30, 2010 at 3:30 PM,  <jrbarnette@chromium.org> wrote:
>> I have some (unimportant) typo notes.
>>
>> More substantively, I have two questions:
>>  * Am I correct in reading that "class BootReason" is unused and  
>> untested?
>>  * Am I correct in reading that "class RecoveryButton" is unused  
>> except for
>>   its unit tests?
>
> Yup. I started down the path with BootReason and RecoveryButton at
> Randall's recommendation, but once I started to explore the firmware
> specification and the platform interfaces, I realized that neither of
> those values are important now for use in cros_boot_mode.  However, I
> didn't see any reason not to keep the code around for extended use --
> even if not in cros_boot_mode itself.
>
Well, my experience with dead code is that it can (and does) create
problems:  Developers inevitably spend time maintaining the dead
code, even though that time can't be paid back with product quality.
Moreover, even with maintenance, the dead code is likely to accumulate
bugs (since effective testing can't be done for code that's never
actually executed).

Witness:  You already have an RFE for this dead code from Chris Sosa,
and the code isn't even integrated yet.  :-)

> I can add a basic unittest on bootreason and let it lie, or if you
> prefer drop both from the CL.  (I'm not picky :)
>
Well, cros_boot_mode is a command line tool, and as such could be
useful to developers on its own.  So, if you want to hook BootReason
and RecoveryButton into main() via additional command line options,
I could happily overlook that no existing script actually uses the
options.  :-)

OTOH, if you think the code isn't worth the work of extra options
to the command, I'd argue it isn't really worth keeping around
"just in case."

Thanks!

> I'll reply to the specific comments inline as I fix them.
>
> Thanks!
> will
>
>>
>>
>>
>> http://codereview.chromium.org/3530001/diff/13001/14002
>> File Makefile (right):
>>
>> http://codereview.chromium.org/3530001/diff/13001/14002#newcode23
>> Makefile:23: # Tests are not build by default.
>> typo: "built"
>>
>> http://codereview.chromium.org/3530001/diff/13001/14015
>> File common.mk (right):
>>
>> http://codereview.chromium.org/3530001/diff/13001/14015#newcode7
>> common.mk:7: # can be built in the source tree or with the build
>> products places elsewhere.
>> typo: "placed"
>>
>> http://codereview.chromium.org/3530001/diff/13001/14015#newcode16
>> common.mk:16: #    $(BUILD)relative/path/target: ...
>> Is this correct?  Makefile uses "$(OUT)path/target:", and there's no
>> BUILD
>> defined in this file (but OUT is).
>>
>> http://codereview.chromium.org/3530001/show
>>

-- jrb

[jrbarnette, 2010-09-30 21:27:10]

On Sep 30, 2010, at 1:51 PM, wad@chromium.org wrote:

> Thanks!
>
> What do you think about an ImageType class? Maybe it could be  
> enabled (or
> disabled) as premptory using something like -i / - 
> use_image_type_first or
> something similar.  I do worry about how much many paths we'll have,  
> but I'm
> cool centralizing it all first :)
>
Right.  To the extent that we have this code at all, it's important
that we distinguish between the run-time environment passed to us
by the firmware, and the build-time options that created the running
image.  So, an ImageType class separate from the BootMode class is
a good idea (but do take note of my earlier cautions regarding dead
code).

Given that we already have clients for the "image mode" test, I
think the case that we want such a feature here is strong; that
said, I'd still recommend that we not add an "image mode" test
until we've also lined up the script changes that delete the
existing test code (meaning, in practice, that the change should
be a separate, later CL).

Thanks!


>
> http://codereview.chromium.org/3530001/diff/13001/14002
> File Makefile (right):
>
> http://codereview.chromium.org/3530001/diff/13001/14002#newcode23
> Makefile:23: # Tests are not build by default.
> On 2010/09/30 20:46:00, sosa wrote:
>> s/build/built
>
> Done.
>
> http://codereview.chromium.org/3530001/diff/13001/14011
> File boot_reason.cc (right):
>
> http://codereview.chromium.org/3530001/diff/13001/14011#newcode38
> boot_reason.cc:38: return kUnsupported;
> On 2010/09/30 20:46:00, sosa wrote:
>> You may want to consider checking /root/.dev_mode for unsupported ...
> i.e. if
>> exists return developer otherwise normal.  This is the case for
> unsupported
>> firmware so we can continue to test this with those devices.
>
> I don't want to pollute the logic in this class with hidden filesystem
> secrets.  This file specifically instruments the Boot Reason code from
> the firmware.  The BootMode front end class provides the  
> centralization
> of this sort of logic.
>
> We can certainly add that in -- if you look at how the bootloader
> override is done, it is similar.  We can add a
>
> ImageType class which will check .dev_mode, .test?, etc and could be
> used to pre-empt further detection or replace unsupported with
> "developer".  That said, there is some conflation between image and  
> mode
> and I'm not sure where it makes sense to draw the line.
>
> For your query, we can test on unsupported devices because they will
> still operate in "normal" mode, but if a password is stamped, it  
> will be
> usable (eg on a test image).  However, we can add a ImageType into the
> mix and start to remove it from being hardcoded everywhere else!
>
> http://codereview.chromium.org/3530001/show

-- jrb

[Will Drewry, 2010-09-30 22:10:35]

On Thu, Sep 30, 2010 at 4:27 PM, Richard Barnette
<jrbarnette@chromium.org> wrote:
> On Sep 30, 2010, at 1:51 PM, wad@chromium.org wrote:
>
>> Thanks!
>>
>> What do you think about an ImageType class? Maybe it could be enabled (or
>> disabled) as premptory using something like -i / -use_image_type_first or
>> something similar.  I do worry about how much many paths we'll have, but
>> I'm
>> cool centralizing it all first :)
>>
> Right.  To the extent that we have this code at all, it's important
> that we distinguish between the run-time environment passed to us
> by the firmware, and the build-time options that created the running
> image.  So, an ImageType class separate from the BootMode class is
> a good idea (but do take note of my earlier cautions regarding dead
> code).

Agreed.

>
> Given that we already have clients for the "image mode" test, I
> think the case that we want such a feature here is strong; that
> said, I'd still recommend that we not add an "image mode" test
> until we've also lined up the script changes that delete the
> existing test code (meaning, in practice, that the change should
> be a separate, later CL).
>

Yep - I'd prefer not to make this cl grow infinitely large :)

I'll drop the recoverybutton, but I may wire up BootReason to a -how
flag.  In particular, it would be nice to know if we rebooted in debug
mode and need to drop kernel preserved ram.  However, that is a whole
other can of worms I don't want to complicate this cl with :).  So
I'll pull BootReason into a private branch and add the -how options
after this cl.


> Thanks!

Thank you!

>
>>
>> http://codereview.chromium.org/3530001/diff/13001/14002
>> File Makefile (right):
>>
>> http://codereview.chromium.org/3530001/diff/13001/14002#newcode23
>> Makefile:23: # Tests are not build by default.
>> On 2010/09/30 20:46:00, sosa wrote:
>>>
>>> s/build/built
>>
>> Done.
>>
>> http://codereview.chromium.org/3530001/diff/13001/14011
>> File boot_reason.cc (right):
>>
>> http://codereview.chromium.org/3530001/diff/13001/14011#newcode38
>> boot_reason.cc:38: return kUnsupported;
>> On 2010/09/30 20:46:00, sosa wrote:
>>>
>>> You may want to consider checking /root/.dev_mode for unsupported ...
>>
>> i.e. if
>>>
>>> exists return developer otherwise normal.  This is the case for
>>
>> unsupported
>>>
>>> firmware so we can continue to test this with those devices.
>>
>> I don't want to pollute the logic in this class with hidden filesystem
>> secrets.  This file specifically instruments the Boot Reason code from
>> the firmware.  The BootMode front end class provides the centralization
>> of this sort of logic.
>>
>> We can certainly add that in -- if you look at how the bootloader
>> override is done, it is similar.  We can add a
>>
>> ImageType class which will check .dev_mode, .test?, etc and could be
>> used to pre-empt further detection or replace unsupported with
>> "developer".  That said, there is some conflation between image and mode
>> and I'm not sure where it makes sense to draw the line.
>>
>> For your query, we can test on unsupported devices because they will
>> still operate in "normal" mode, but if a password is stamped, it will be
>> usable (eg on a test image).  However, we can add a ImageType into the
>> mix and start to remove it from being hardcoded everywhere else!
>>
>> http://codereview.chromium.org/3530001/show
>
> -- jrb
>
>

[jrbarnette, 2010-09-30 23:19:48]

I have a huge heapin' helpin' of style nits and comment readability
suggestions.  I haven't found any bugs yet.

http://codereview.chromium.org/3530001/diff/22001/23006
File boot_mode.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23006#newcode55
boot_mode.cc:55: switch(developer_switch_->value()) {
nit, space after 'switch' keyword'

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/3530001/diff/22001/23007
File boot_mode.h (right):

http://codereview.chromium.org/3530001/diff/22001/23007#newcode32
boot_mode.h:32: virtual bool Initialize(bool unsupported_is_developer, bool
use_bootloader);
There's no comment explaining the return value for this function, its
meaning isn't immediately obvious, and the return value is ignored by
all the callers.  I don't know what the right fix is, but the status quo has
three strikes against it.

http://codereview.chromium.org/3530001/diff/22001/23007#newcode32
boot_mode.h:32: virtual bool Initialize(bool unsupported_is_developer, bool
use_bootloader);
"... I don't know what the right fix is ...":  I take it back; the function
implementation returns true in every code path, so the return value is
apparently worthless.  It looks like the right fix is to make this function
void.

http://codereview.chromium.org/3530001/diff/22001/23007#newcode35
boot_mode.h:35: { return (mode() >= 0 ? kBootModeText[mode()] : "unsupported");
}
nit:  Here and in a lot of other places, there are two formats used for these
immediate definitions:
  <fn declaration>
    { <fn body>; }
and also
  <fn declaration> {
    <fn body>;
  }

The style guide seems to prescribe the latter format (and I rather prefer it):
   
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/3530001/diff/22001/23008
File boot_mode_main.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23008#newcode39
boot_mode_main.cc:39: "It will substring match unless -strict_match is
supplied:\n"
Perhaps this help text could be slightly clarified?  Perhaps says something
about matching leading substrings?  As written, it's not immediately clear
that while "-m developer" matches "developer recovery", "-m recovery"
doesn't match.

http://codereview.chromium.org/3530001/diff/22001/23013
File bootloader_type.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23013#newcode26
bootloader_type.cc:26: // The first match will be honorred first.
typo: "honored"

Mentioning here or below that these strings are options on the kernel
command line would have helped me understand better during my
first pass through the code.  A variable name that suggested this fact
could also help, but I don't have any bright ideas as to what that name
might be.

A blank line before line 25 would make this more visually appealing
to me, but I see that the Google style guide discourages vertical white
space.  :-(

The comment text here is actually confusing.  How about just saying
something like the indexes in the two arrays need to correspond?

http://codereview.chromium.org/3530001/diff/22001/23013#newcode37
bootloader_type.cc:37: int BootloaderType::Process(const char *contents, size_t
length) {
A comment round about here (or maybe above) that this code is scanning the
kernel command line would have saved me some flipping back and forth
between files when I was trying to figure this out.

http://codereview.chromium.org/3530001/diff/22001/23014
File bootloader_type.h (right):

http://codereview.chromium.org/3530001/diff/22001/23014#newcode6
bootloader_type.h:6: // reason" and returns an integer or a human-readable,
script-parseable textual
"boot reason":  Should this be "boot type"?  Also, something that describes that
the firmware supplies this on the kernel command line would help.

http://codereview.chromium.org/3530001/diff/22001/23014#newcode9
bootloader_type.h:9: // N.b., the boot reason alone cannot determine if the
running system is in
nit/typo: "N.B."

http://codereview.chromium.org/3530001/diff/22001/23014#newcode9
bootloader_type.h:9: // N.b., the boot reason alone cannot determine if the
running system is in
Looking more closely at this comment block, I wonder whether it might
need some scrubbing.  The comments seem more appropriate to the
BootReason class, and they mention a non-existent FirmwareSwitch().

http://codereview.chromium.org/3530001/diff/22001/23022
File platform_reader.h (right):

http://codereview.chromium.org/3530001/diff/22001/23022#newcode6
platform_reader.h:6: // from a file and assigned it to a PlatformValue which is
The term PlatformValue appears here and in other comments, but there's
no such identifier in the source.

http://codereview.chromium.org/3530001/diff/22001/23023
File platform_switch.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23023#newcode7
platform_switch.cc:7: // implementation-specific.
This comment could use some scrubbing; at the very least it should
correctly name which class it implements.  :-)

http://codereview.chromium.org/3530001/diff/22001/23023#newcode9
platform_switch.cc:9: //#define FIRMWARE_UTILS_PLATFORM_READER_H_
... and as long as you're changing this file, you could delete these lines,
too.  :-)

http://codereview.chromium.org/3530001/diff/22001/23024
File platform_switch.h (right):

http://codereview.chromium.org/3530001/diff/22001/23024#newcode6
platform_switch.h:6: // from a file and assigned it to a PlatformValue which is
typo "assign"

http://codereview.chromium.org/3530001/diff/22001/23024#newcode7
platform_switch.h:7: // implementation-specific.
This text could be made more useful.  When I read this, the following facts
were most pertinent to my understanding:
  * The switch values are read from the "CHSW" file.
  * The file format is an integer with bit settings for various switch
    conditions detected by the firmware.
  * This is an abstract class (or whatever it is that C++ calls it).

The term PlatformValue is mentioned in this comment, but appears
nowhere in the source.

http://codereview.chromium.org/3530001/diff/22001/23024#newcode27
platform_switch.h:27: virtual size_t max_size() const { return 3; }
What's the derivation of the "3" magic number?  If it's the same as
the sizeof("-1") in active_main_firmware.h, I much prefer that solution.

[fes, 2010-10-01 21:51:23]

Minor nits + what seems to be an off-by-one.

http://codereview.chromium.org/3530001/diff/22001/23004
File active_main_firmware.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23004#newcode26
active_main_firmware.cc:26:
sizeof(kActiveMainFirmwareText)/sizeof(*kActiveMainFirmwareText);
There are a few places where you have no space between binary operators, such as
above, and some where there is space.

http://codereview.chromium.org/3530001/diff/22001/23011
File boot_reason.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23011#newcode30
boot_reason.cc:30: sizeof(kBootReasonText)/sizeof(*kBootReasonText);
Another no space binary operator.

http://codereview.chromium.org/3530001/diff/22001/23018
File helpers.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23018#newcode23
helpers.cc:23: return 0;
Indented one extra space.

http://codereview.chromium.org/3530001/diff/22001/23018#newcode26
helpers.cc:26: buf[bytes_read] = '\0';
I believe you'll overflow here if bytes_read == max_bytes, as the call site that
I found (PlatformReader) doesn't allocate +1 bytes.  Perhaps both:
if (!max)
  return 0;
and
::fread(buf, 1, max_bytes - 1, fp);

[jrbarnette, 2010-10-01 22:52:51]

I've gone through the unit tests now.  At this point I've looked at
all the live files (though not, apparently, as thoroughly as fes :-) ),
but I'll hold off on the dead code files until I hear what their future
holds.

http://codereview.chromium.org/3530001/diff/22001/23010
File boot_mode_unittest.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23010#newcode45
boot_mode_unittest.cc:45: void UpdateFiles(int chsw, int fw, const char
*cmdline) {
A few lines of comment detailing this function could be quite useful;
note also comments below.

http://codereview.chromium.org/3530001/diff/22001/23010#newcode98
boot_mode_unittest.cc:98: 
Looking below, I see missing coverage for the following cases:
  * no coverage for unsupported_is_developer == true
  * no coverage for active FW=B
  * no coverage for cros_efi or cros_legacy bootloader_type

http://codereview.chromium.org/3530001/diff/22001/23010#newcode99
boot_mode_unittest.cc:99: TEST_F(BootModeTest, NoFilesUseBootloader) {
Here and for the other tests, a one to two line comment detailing the
assertion being tested would help.

An alternative to commenting each test case might also be comments
on the definition of UpdateFiles(), since that's where the action is.

[Will Drewry, 2010-10-04 20:11:27]

I've attempted to address all the comments - from nits to style to bugs and
added some more testing.

PTAL!

Thanks for all the great feedback!

http://codereview.chromium.org/3530001/diff/22001/23004
File active_main_firmware.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23004#newcode26
active_main_firmware.cc:26:
sizeof(kActiveMainFirmwareText)/sizeof(*kActiveMainFirmwareText);
On 2010/10/01 21:51:24, fes wrote:
> There are a few places where you have no space between binary operators, such
as
> above, and some where there is space.

Done.

http://codereview.chromium.org/3530001/diff/22001/23006
File boot_mode.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23006#newcode55
boot_mode.cc:55: switch(developer_switch_->value()) {
On 2010/09/30 23:19:48, jrbarnette wrote:
> nit, space after 'switch' keyword'
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23007
File boot_mode.h (right):

http://codereview.chromium.org/3530001/diff/22001/23007#newcode32
boot_mode.h:32: virtual bool Initialize(bool unsupported_is_developer, bool
use_bootloader);
On 2010/09/30 23:19:48, jrbarnette wrote:
> "... I don't know what the right fix is ...":  I take it back; the function
> implementation returns true in every code path, so the return value is
> apparently worthless.  It looks like the right fix is to make this function
> void.
> 

Yeah - I was trying to decide if I wanted to propagate any failure out.  But
instead, I'm favoring failure == unsupported since most cases map that way. 
I'll fix this up to match.

http://codereview.chromium.org/3530001/diff/22001/23007#newcode32
boot_mode.h:32: virtual bool Initialize(bool unsupported_is_developer, bool
use_bootloader);
On 2010/09/30 23:19:48, jrbarnette wrote:
> There's no comment explaining the return value for this function, its
> meaning isn't immediately obvious, and the return value is ignored by
> all the callers.  I don't know what the right fix is, but the status quo has
> three strikes against it.
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23007#newcode35
boot_mode.h:35: { return (mode() >= 0 ? kBootModeText[mode()] : "unsupported");
}
On 2010/09/30 23:19:48, jrbarnette wrote:
> nit:  Here and in a lot of other places, there are two formats used for these
> immediate definitions:
>   <fn declaration>
>     { <fn body>; }
> and also
>   <fn declaration> {
>     <fn body>;
>   }
> 
> The style guide seems to prescribe the latter format (and I rather prefer it):
>    
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23008
File boot_mode_main.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23008#newcode39
boot_mode_main.cc:39: "It will substring match unless -strict_match is
supplied:\n"
On 2010/09/30 23:19:48, jrbarnette wrote:
> Perhaps this help text could be slightly clarified?  Perhaps says something
> about matching leading substrings?  As written, it's not immediately clear
> that while "-m developer" matches "developer recovery", "-m recovery"
> doesn't match.
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23010
File boot_mode_unittest.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23010#newcode45
boot_mode_unittest.cc:45: void UpdateFiles(int chsw, int fw, const char
*cmdline) {
On 2010/10/01 22:52:51, jrbarnette wrote:
> A few lines of comment detailing this function could be quite useful;
> note also comments below.
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23010#newcode98
boot_mode_unittest.cc:98: 
On 2010/10/01 22:52:51, jrbarnette wrote:
> Looking below, I see missing coverage for the following cases:
>   * no coverage for unsupported_is_developer == true
>   * no coverage for active FW=B
>   * no coverage for cros_efi or cros_legacy bootloader_type

done.

For _efi and _legacy, I've added some bootloader_type tests too.

http://codereview.chromium.org/3530001/diff/22001/23010#newcode99
boot_mode_unittest.cc:99: TEST_F(BootModeTest, NoFilesUseBootloader) {
On 2010/10/01 22:52:51, jrbarnette wrote:
> Here and for the other tests, a one to two line comment detailing the
> assertion being tested would help.
> 
> An alternative to commenting each test case might also be comments
> on the definition of UpdateFiles(), since that's where the action is.
> 
> 

Maybe the updatefiles comment plus the test names will help. If not I cna expand
upon it.

http://codereview.chromium.org/3530001/diff/22001/23011
File boot_reason.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23011#newcode30
boot_reason.cc:30: sizeof(kBootReasonText)/sizeof(*kBootReasonText);
On 2010/10/01 21:51:24, fes wrote:
> Another no space binary operator.

Done.

http://codereview.chromium.org/3530001/diff/22001/23013
File bootloader_type.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23013#newcode37
bootloader_type.cc:37: int BootloaderType::Process(const char *contents, size_t
length) {
On 2010/09/30 23:19:48, jrbarnette wrote:
> A comment round about here (or maybe above) that this code is scanning the
> kernel command line would have saved me some flipping back and forth
> between files when I was trying to figure this out.
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23014
File bootloader_type.h (right):

http://codereview.chromium.org/3530001/diff/22001/23014#newcode6
bootloader_type.h:6: // reason" and returns an integer or a human-readable,
script-parseable textual
This comments was completely wrong. Fixed!

http://codereview.chromium.org/3530001/diff/22001/23014#newcode9
bootloader_type.h:9: // N.b., the boot reason alone cannot determine if the
running system is in
On 2010/09/30 23:19:48, jrbarnette wrote:
> Looking more closely at this comment block, I wonder whether it might
> need some scrubbing.  The comments seem more appropriate to the
> BootReason class, and they mention a non-existent FirmwareSwitch().

Done.

http://codereview.chromium.org/3530001/diff/22001/23014#newcode9
bootloader_type.h:9: // N.b., the boot reason alone cannot determine if the
running system is in
On 2010/09/30 23:19:48, jrbarnette wrote:
> nit/typo: "N.B."

Done.

http://codereview.chromium.org/3530001/diff/22001/23018
File helpers.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23018#newcode23
helpers.cc:23: return 0;
On 2010/10/01 21:51:24, fes wrote:
> Indented one extra space.

Done.

http://codereview.chromium.org/3530001/diff/22001/23018#newcode26
helpers.cc:26: buf[bytes_read] = '\0';
On 2010/10/01 21:51:24, fes wrote:
> I believe you'll overflow here if bytes_read == max_bytes, as the call site
that
> I found (PlatformReader) doesn't allocate +1 bytes.  Perhaps both:
> if (!max)
>   return 0;
> and
> ::fread(buf, 1, max_bytes - 1, fp);

Done.

http://codereview.chromium.org/3530001/diff/22001/23022
File platform_reader.h (right):

http://codereview.chromium.org/3530001/diff/22001/23022#newcode6
platform_reader.h:6: // from a file and assigned it to a PlatformValue which is
On 2010/09/30 23:19:48, jrbarnette wrote:
> The term PlatformValue appears here and in other comments, but there's
> no such identifier in the source.
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23023
File platform_switch.cc (right):

http://codereview.chromium.org/3530001/diff/22001/23023#newcode7
platform_switch.cc:7: // implementation-specific.
On 2010/09/30 23:19:48, jrbarnette wrote:
> This comment could use some scrubbing; at the very least it should
> correctly name which class it implements.  :-)
> 
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23023#newcode9
platform_switch.cc:9: //#define FIRMWARE_UTILS_PLATFORM_READER_H_
On 2010/09/30 23:19:48, jrbarnette wrote:
> ... and as long as you're changing this file, you could delete these lines,
> too.  :-)
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23024
File platform_switch.h (right):

http://codereview.chromium.org/3530001/diff/22001/23024#newcode6
platform_switch.h:6: // from a file and assigned it to a PlatformValue which is
On 2010/09/30 23:19:48, jrbarnette wrote:
> typo "assign"

Done.

http://codereview.chromium.org/3530001/diff/22001/23024#newcode7
platform_switch.h:7: // implementation-specific.
On 2010/09/30 23:19:48, jrbarnette wrote:
> This text could be made more useful.  When I read this, the following facts
> were most pertinent to my understanding:
>   * The switch values are read from the "CHSW" file.
>   * The file format is an integer with bit settings for various switch
>     conditions detected by the firmware.
>   * This is an abstract class (or whatever it is that C++ calls it).
> 
> The term PlatformValue is mentioned in this comment, but appears
> nowhere in the source.
> 

Done.

http://codereview.chromium.org/3530001/diff/22001/23024#newcode27
platform_switch.h:27: virtual size_t max_size() const { return 3; }
On 2010/09/30 23:19:48, jrbarnette wrote:
> What's the derivation of the "3" magic number?  If it's the same as
> the sizeof("-1") in active_main_firmware.h, I much prefer that solution.
> 
> 

Done.

[fes, 2010-10-04 20:16:37]

LGTM

[Will Drewry, 2010-10-04 20:52:39]

On 2010/10/04 20:16:37, fes wrote:
> LGTM


Thanks!

@jrbarnette: I'll wait for your LGTM too given the extensive feedback.  Thanks
again!

will

[jrbarnette, 2010-10-05 15:51:06]

Follow-up comments covering the code but not the unit tests.

I found a bug, so now I'm going to burden you with further comment
nits.  :-)

I'm continuing to review the unit tests.

http://codereview.chromium.org/3530001/diff/7002/41008
File boot_mode_main.cc (right):

http://codereview.chromium.org/3530001/diff/7002/41008#newcode114
boot_mode_main.cc:114: // "developer recovery".  We shorten the comparision by
using the
<sigh> A typo I didn't catch before:  "comparison".

http://codereview.chromium.org/3530001/diff/7002/41011
File bootloader_type.cc (right):

http://codereview.chromium.org/3530001/diff/7002/41011#newcode46
bootloader_type.cc:46: if (*end == '\0' || *end == ' ')
I've just realized that this check won't enforce that the start of the string is
also blank delimited.  So, I believe this will match something like
"no_cros_secure" as well as "cros_secure".

http://codereview.chromium.org/3530001/diff/7002/41012
File bootloader_type.h (right):

http://codereview.chromium.org/3530001/diff/7002/41012#newcode47
bootloader_type.h:47: // the matching array index (or kUnsupported).
This comment is good; For my money it would add more value if it were in
bootloader_type.cc over the function definition, instead of here.

http://codereview.chromium.org/3530001/diff/7002/41016
File developer_switch.h (right):

http://codereview.chromium.org/3530001/diff/7002/41016#newcode5
developer_switch.h:5: // Concrete class for DeveloperSwitch
Missed it before:  this comment could be more useful.

http://codereview.chromium.org/3530001/diff/7002/41018
File helpers.cc (right):

http://codereview.chromium.org/3530001/diff/7002/41018#newcode7
helpers.cc:7: // implementation-specific.
Missed this before:  This comment appears to describe a different file.

[Will Drewry, 2010-10-05 16:11:23]

Bug fixed, unittest added, and comments addressed, I hope!

PTAL :)

thanks!
will

http://codereview.chromium.org/3530001/diff/7002/41008
File boot_mode_main.cc (right):

http://codereview.chromium.org/3530001/diff/7002/41008#newcode114
boot_mode_main.cc:114: // "developer recovery".  We shorten the comparision by
using the
On 2010/10/05 15:51:06, jrbarnette wrote:
> <sigh> A typo I didn't catch before:  "comparison".
> 

Done.

http://codereview.chromium.org/3530001/diff/7002/41011
File bootloader_type.cc (right):

http://codereview.chromium.org/3530001/diff/7002/41011#newcode46
bootloader_type.cc:46: if (*end == '\0' || *end == ' ')
On 2010/10/05 15:51:06, jrbarnette wrote:
> I've just realized that this check won't enforce that the start of the string
is
> also blank delimited.  So, I believe this will match something like
> "no_cros_secure" as well as "cros_secure".
> 

Nice catch.  Added a unit test case for leading and trailing boundaries and
added a fix.

http://codereview.chromium.org/3530001/diff/7002/41012
File bootloader_type.h (right):

http://codereview.chromium.org/3530001/diff/7002/41012#newcode47
bootloader_type.h:47: // the matching array index (or kUnsupported).
On 2010/10/05 15:51:06, jrbarnette wrote:
> This comment is good; For my money it would add more value if it were in
> bootloader_type.cc over the function definition, instead of here.
> 

I agree, but style guide says here.  I'll add a referencing comment just to help
a coder out :)

http://codereview.chromium.org/3530001/diff/7002/41016
File developer_switch.h (right):

http://codereview.chromium.org/3530001/diff/7002/41016#newcode5
developer_switch.h:5: // Concrete class for DeveloperSwitch
On 2010/10/05 15:51:06, jrbarnette wrote:
> Missed it before:  this comment could be more useful.
> 

Done.

http://codereview.chromium.org/3530001/diff/7002/41018
File helpers.cc (right):

http://codereview.chromium.org/3530001/diff/7002/41018#newcode7
helpers.cc:7: // implementation-specific.
On 2010/10/05 15:51:06, jrbarnette wrote:
> Missed this before:  This comment appears to describe a different file.
> 
Done.

Also updated the .h with slightly better info.

[jrbarnette, 2010-10-05 16:27:01]

My comments on the unit tests below.  I have nothing specific regarding
any single line in bootloader_type_unittests.cc, but I note that the
code does very deftly avoid discovering what would happen with a
command option of the form "no_cros_secure".

http://codereview.chromium.org/3530001/diff/7002/41017
File developer_switch_unittest.cc (right):

http://codereview.chromium.org/3530001/diff/7002/41017#newcode42
developer_switch_unittest.cc:42: 
Looking at this (and reflecting on helpers.cc), I'd be interested to know
what happens with the text "100000" as file contents, not to mention
"100032".  Not that I think that there's a bug, mind you, I'm just sayin'.

http://codereview.chromium.org/3530001/diff/7002/41017#newcode80
developer_switch_unittest.cc:80:
EXPECT_EQ(file_util::WriteFile(FilePath(switch_file_path_), "", 3), 3);
Hmmm...  sizeof("") != 3...

[jrbarnette, 2010-10-05 17:10:55]

On 2010/10/05 16:11:23, Will Drewry wrote:
[ ... ]
> On 2010/10/05 15:51:06, jrbarnette wrote:
> > This comment is good; For my money it would add more value if it were in
> > bootloader_type.cc over the function definition, instead of here.
> > 
> 
> I agree, but style guide says here.  I'll add a referencing comment just to
help
> a coder out :)
> 
<sigh> Well, Charles Dickens seems to have had the most apropos insight:
    http://www.bartleby.com/73/1002.html

I can't say as there's much point going toe-to-toe with City Hall over this
issue, though.  :-(  (I also don't recall as Dickens had any better luck in his
day).

[Will Drewry, 2010-10-05 19:11:40]

I changed the read_file behavior slightly and updated the unittests. 

PTAL :)

http://codereview.chromium.org/3530001/diff/7002/41017
File developer_switch_unittest.cc (right):

http://codereview.chromium.org/3530001/diff/7002/41017#newcode42
developer_switch_unittest.cc:42: 
On 2010/10/05 16:27:01, jrbarnette wrote:
> Looking at this (and reflecting on helpers.cc), I'd be interested to know
> what happens with the text "100000" as file contents, not to mention
> "100032".  Not that I think that there's a bug, mind you, I'm just sayin'.
> 

Done. 

So there is a known bug around truncation here.  I originally figured that was
fine since we define both interfaces.  However, the more I looked at it, the
more I worried that we may arbitrarily extend the platform supplied value from a
max of 16-bits to 32-bits (or beyond).  So I've reworked things a bit to ensure
that read_file reads the whole file or fails.  This will map inconsistent
platform changes to kUnsupported which will be slightly easier to diagnose :/

http://codereview.chromium.org/3530001/diff/7002/41017#newcode80
developer_switch_unittest.cc:80:
EXPECT_EQ(file_util::WriteFile(FilePath(switch_file_path_), "", 3), 3);
On 2010/10/05 16:27:01, jrbarnette wrote:
> Hmmm...  sizeof("") != 3...
> 

Done.

[jrbarnette, 2010-10-05 19:45:18]

Ah, this appears to be the end of it.

LGTM!