add an upstart script: vpd-log.conf

add an upstart script: vpd-log.conf

This script is going to dump VPD information into /var/log/vpd_2.0.txt to make
user read them in chrome://system.

VPD (Vital Product Data) are written into BIOS flash during factory.
It contains machine-specific information, e.g., mother board serial number,
MAC address, 3G module serial number...
Those information are important when user sends machine back to service center.

Change-Id: I3e555900e8711e99cc02c67b5b6e41647ec0e6c4

BUG=chrome-os-partner:1462
TEST=Tested on Mario.

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

[Chris Masone, 2011-02-24 06:25:34]

http://codereview.chromium.org/6581026/diff/1/vpd-log.conf
File vpd-log.conf (right):

http://codereview.chromium.org/6581026/diff/1/vpd-log.conf#newcode10
vpd-log.conf:10: start on stopped boot-complete
There may be a better time to trigger this, and a way to make it so it just
happens once per boot -- not every time the login screen comes back up.  Please
add jrbarnette as a reviewer and ask him to chime in.

http://codereview.chromium.org/6581026/diff/1/vpd-log.conf#newcode13
vpd-log.conf:13: task
Heh.  I recently found out from jrbarnette that we've been using "task" wrong
all this time :-/

This should actually just be taken out.  As long as you don't say "respawn",
it'll just run once per "boot-complete"

[Chris Masone, 2011-02-24 06:27:58]

Speaking of which :-)
http://code.google.com/p/chromium-os/issues/detail?id=12487


On 2011/02/24 06:25:34, Chris Masone wrote:
> http://codereview.chromium.org/6581026/diff/1/vpd-log.conf
> File vpd-log.conf (right):
> 
> http://codereview.chromium.org/6581026/diff/1/vpd-log.conf#newcode10
> vpd-log.conf:10: start on stopped boot-complete
> There may be a better time to trigger this, and a way to make it so it just
> happens once per boot -- not every time the login screen comes back up. 
Please
> add jrbarnette as a reviewer and ask him to chime in.
> 
> http://codereview.chromium.org/6581026/diff/1/vpd-log.conf#newcode13
> vpd-log.conf:13: task
> Heh.  I recently found out from jrbarnette that we've been using "task" wrong
> all this time :-/
> 
> This should actually just be taken out.  As long as you don't say "respawn",
> it'll just run once per "boot-complete"

[Louis, 2011-02-24 10:21:55]

Chris, thanks for the review. Please see my comments inlined.

http://codereview.chromium.org/6581026/diff/1/vpd-log.conf
File vpd-log.conf (right):

http://codereview.chromium.org/6581026/diff/1/vpd-log.conf#newcode10
vpd-log.conf:10: start on stopped boot-complete
I intended putting this after boot-complete so that it doesn't affect the boot
time calculation.

However, since the VPD information is not changed frequently, I can modify the
script so that it won't dump VPD if a cached file exists.

See incoming changes this CL.

On 2011/02/24 06:25:34, Chris Masone wrote:
> There may be a better time to trigger this, and a way to make it so it just
> happens once per boot -- not every time the login screen comes back up. 
Please
> add jrbarnette as a reviewer and ask him to chime in.

http://codereview.chromium.org/6581026/diff/1/vpd-log.conf#newcode13
vpd-log.conf:13: task
On 2011/02/24 06:25:34, Chris Masone wrote:
> Heh.  I recently found out from jrbarnette that we've been using "task" wrong
> all this time :-/
> 
> This should actually just be taken out.  As long as you don't say "respawn",
> it'll just run once per "boot-complete"

Done.

[jrbarnette, 2011-02-24 17:34:17]

OK, I've added my comments.  Mostly style nits now, 'cause cmasone
caught the substantive issues.

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf
File vpd-log.conf (right):

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode8
vpd-log.conf:8: # partitions.
I've got upcoming changes to try and standardize the style of the
comment header here.  This is the style I want to encourage:

    description     "... brief description ..."
    author          "chromium-os-dev@chromium.org"

    # Description of the purpose of the job, and any non-obvious
    # reasons behind the start and stop conditions.  This shouldn't
    # just repeat the 'description' stanza; if that stanza says
    # everything, leave out these comments.
    start on stopped boot-complete

I'd rather not see phrases that repeat the obvious, like "when boot
reaches login prompt, ...":  The 'start on' stanza makes that clear,
so the comment is too much like "a++; /* increment a by 1 */".
Important things to call out regarding the start conditions would
be if there's some important reason for starting after login other
than the conventional performance rationale, like a hidden
dependency.

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode18
vpd-log.conf:18: touch $VPD_2_0
This is unnecessary; redirection onto the file below will create it.

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode20
vpd-log.conf:20: if [ -x /usr/sbin/vpd -a -x /usr/sbin/flashrom ]; then
I think this test is pointless for various reasons:
  * Given this job needs them, we shouldn't consider vpd
    or flashrom to be optional, so we shouldn't need to test
    for their presence.
  * This test will prevent vpd from running simply because
    of a missing flashrom, and vice versa.  That's not as robust
    as I'd like to see.

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode23
vpd-log.conf:23: vpd -f "$TMP" -i "RW VPD" -l || echo "RW VPD execute error."
It seems like the logic above could be replaced by something more
like this:
    if flashrom -r $TMP >/dev/null ; then
        vpd -f ...  "RO ..." ...
        vpd -f ...  "RW ..." ...
    fi
    rm -f $TMP

Also, I'd like to see consistent use of quotes around $TMP - either
always used, or always omitted (in this context, I prefer always
omitted, but consistency is much more important than the
particular usage.

[Chris Masone, 2011-02-24 18:22:34]

Thanks, Louis.  This LGTM with respect to my comments, but please do iterate
with Richard to finish this off.

[Louis, 2011-02-25 08:50:12]

Richard,

Your suggestions make very sense to me. I've rewrite my CL according to your
suggestions. Please kindly take a review on it.

Thanks.

Louis

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf
File vpd-log.conf (right):

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode8
vpd-log.conf:8: # partitions.
On 2011/02/24 17:34:17, jrbarnette wrote:
> I've got upcoming changes to try and standardize the style of the
> comment header here.  This is the style I want to encourage:
> 
>     description     "... brief description ..."
>     author          "chromium-os-dev@chromium.org"
> 
>     # Description of the purpose of the job, and any non-obvious
>     # reasons behind the start and stop conditions.  This shouldn't
>     # just repeat the 'description' stanza; if that stanza says
>     # everything, leave out these comments.
>     start on stopped boot-complete
> 
> I'd rather not see phrases that repeat the obvious, like "when boot
> reaches login prompt, ...":  The 'start on' stanza makes that clear,
> so the comment is too much like "a++; /* increment a by 1 */".
> Important things to call out regarding the start conditions would
> be if there's some important reason for starting after login other
> than the conventional performance rationale, like a hidden
> dependency.
> 

Done.

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode18
vpd-log.conf:18: touch $VPD_2_0
On 2011/02/24 17:34:17, jrbarnette wrote:
> This is unnecessary; redirection onto the file below will create it.

Done.

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode20
vpd-log.conf:20: if [ -x /usr/sbin/vpd -a -x /usr/sbin/flashrom ]; then
On 2011/02/24 17:34:17, jrbarnette wrote:
> I think this test is pointless for various reasons:
>   * Given this job needs them, we shouldn't consider vpd
>     or flashrom to be optional, so we shouldn't need to test
>     for their presence.
>   * This test will prevent vpd from running simply because
>     of a missing flashrom, and vice versa.  That's not as robust
>     as I'd like to see.

Done.

http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode23
vpd-log.conf:23: vpd -f "$TMP" -i "RW VPD" -l || echo "RW VPD execute error."
On 2011/02/24 17:34:17, jrbarnette wrote:
> It seems like the logic above could be replaced by something more
> like this:
>     if flashrom -r $TMP >/dev/null ; then
>         vpd -f ...  "RO ..." ...
>         vpd -f ...  "RW ..." ...
>     fi
>     rm -f $TMP
> 
> Also, I'd like to see consistent use of quotes around $TMP - either
> always used, or always omitted (in this context, I prefer always
> omitted, but consistency is much more important than the
> particular usage.

Done.

[jrbarnette, 2011-02-25 16:44:15]

On 2011/02/25 08:50:12, Louis wrote:
> Richard,
> 
> Your suggestions make very sense to me. I've rewrite my CL according to your
> suggestions. Please kindly take a review on it.
> 
Great!  LGTM!

Thanks!


> Thanks.
> 
> Louis
> 
> http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf
> File vpd-log.conf (right):
> 
> http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode8
> vpd-log.conf:8: # partitions.
> On 2011/02/24 17:34:17, jrbarnette wrote:
> > I've got upcoming changes to try and standardize the style of the
> > comment header here.  This is the style I want to encourage:
> > 
> >     description     "... brief description ..."
> >     author          "chromium-os-dev@chromium.org"
> > 
> >     # Description of the purpose of the job, and any non-obvious
> >     # reasons behind the start and stop conditions.  This shouldn't
> >     # just repeat the 'description' stanza; if that stanza says
> >     # everything, leave out these comments.
> >     start on stopped boot-complete
> > 
> > I'd rather not see phrases that repeat the obvious, like "when boot
> > reaches login prompt, ...":  The 'start on' stanza makes that clear,
> > so the comment is too much like "a++; /* increment a by 1 */".
> > Important things to call out regarding the start conditions would
> > be if there's some important reason for starting after login other
> > than the conventional performance rationale, like a hidden
> > dependency.
> > 
> 
> Done.
> 
> http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode18
> vpd-log.conf:18: touch $VPD_2_0
> On 2011/02/24 17:34:17, jrbarnette wrote:
> > This is unnecessary; redirection onto the file below will create it.
> 
> Done.
> 
> http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode20
> vpd-log.conf:20: if [ -x /usr/sbin/vpd -a -x /usr/sbin/flashrom ]; then
> On 2011/02/24 17:34:17, jrbarnette wrote:
> > I think this test is pointless for various reasons:
> >   * Given this job needs them, we shouldn't consider vpd
> >     or flashrom to be optional, so we shouldn't need to test
> >     for their presence.
> >   * This test will prevent vpd from running simply because
> >     of a missing flashrom, and vice versa.  That's not as robust
> >     as I'd like to see.
> 
> Done.
> 
> http://codereview.chromium.org/6581026/diff/5001/vpd-log.conf#newcode23
> vpd-log.conf:23: vpd -f "$TMP" -i "RW VPD" -l || echo "RW VPD execute error."
> On 2011/02/24 17:34:17, jrbarnette wrote:
> > It seems like the logic above could be replaced by something more
> > like this:
> >     if flashrom -r $TMP >/dev/null ; then
> >         vpd -f ...  "RO ..." ...
> >         vpd -f ...  "RW ..." ...
> >     fi
> >     rm -f $TMP
> > 
> > Also, I'd like to see consistent use of quotes around $TMP - either
> > always used, or always omitted (in this context, I prefer always
> > omitted, but consistency is much more important than the
> > particular usage.
> 
> Done.