Fix factory installer for ARM platforms

Fix factory installer for ARM platforms

R=nsanders@chromium.org
BUG=chromium-os:12192
TEST=run factory install successfully

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

[Che-Liang Chiou, 2011-03-29 12:30:40]

Hi Hung-Te,

I hear that Nick is on vacation now. Would you mind helping me review this CL?

[Hung-Te, 2011-03-29 12:57:34]

Basically looks fine, but I have two small questions and would like your
confirm.

http://codereview.chromium.org/6708106/diff/1/factory_install.sh
File factory_install.sh (right):

http://codereview.chromium.org/6708106/diff/1/factory_install.sh#newcode92
factory_install.sh:92: while ! ifconfig eth0 | grep -q "inet addr" && \
Why do we want two inet devices?
And, you don't need the trailing "\" since you have "&&".

http://codereview.chromium.org/6708106/diff/1/factory_install.sh#newcode119
factory_install.sh:119: elif [ $(uname -m ) = "armv7l" ]; then
So we only support v71?

[Nick Sanders, 2011-03-30 00:10:44]

lgtm

http://codereview.chromium.org/6708106/diff/1/factory_install.sh
File factory_install.sh (right):

http://codereview.chromium.org/6708106/diff/1/factory_install.sh#newcode92
factory_install.sh:92: while ! ifconfig eth0 | grep -q "inet addr" && \
This indicates, "loop until either eth0 or eth1 is found" correct? I guess ARM
labels usb eth as eth1 for some reason?

http://codereview.chromium.org/6708106/diff/1/factory_install.sh#newcode121
factory_install.sh:121: else
This has been an annoying problem in general:
http://code.google.com/p/chromium-os/issues/detail?id=10680
http://code.google.com/p/chrome-os-partner/issues/detail?id=3023

Probably doesn't need top be fixed this CL

[Nick Sanders, 2011-03-30 00:11:25]

That is, lgtm, with hungte's style nits fixed.

[Hung-Te, 2011-03-30 02:33:42]

LGTM after Nick's comments.

But please comment in the code for why you want to try both eth0 and eth1, so we
can know if we must extend that range or fix other things in the future.

[Che-Liang Chiou, 2011-03-30 08:57:44]

Hi Nick and Hung-Te,

Based on a discussion with Hung-Te, Rong, and Daniel, I rewrite the part of
probing ethernet interface.

I also accidentally found that test "[ -b /dev/sda ]" is not enough because it
is possible that there is a /dev/sda on Seaboard.

Please take another look.

http://codereview.chromium.org/6708106/diff/1/factory_install.sh
File factory_install.sh (right):

http://codereview.chromium.org/6708106/diff/1/factory_install.sh#newcode92
factory_install.sh:92: while ! ifconfig eth0 | grep -q "inet addr" && \
On 2011/03/30 00:10:44, Nick Sanders wrote:
> This indicates, "loop until either eth0 or eth1 is found" correct? I guess ARM
> labels usb eth as eth1 for some reason?

I changed this part, and hope that it would be more robust to naming of network
interfaces.

[Hung-Te, 2011-03-30 09:17:52]

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh
File factory_install.sh (right):

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh#newcode74
factory_install.sh:74: local candidates=$(ifconfig | grep 'Link encap:Ethernet'
| cut -d ' ' -f 1)
Please quote the $() to "$()", if you use "local".

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh#newcode77
factory_install.sh:77: if ! iw $candidate info > /dev/null 2>&1; then
>/dev/null, no need for the space.

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh#newcode110
factory_install.sh:110: ETHERNET_INTERFACES=$(list_ethernet_interface)
Please do this inside the while-loop, because operator may attach ethernet
during the loop.

[Nick Sanders, 2011-03-30 09:45:33]

W/ updated comment and hungte's fixes, i'm ok with it. Hungte should sign off
also

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh
File factory_install.sh (right):

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh#newcode105
factory_install.sh:105: # is by plugging in a ethernet cable.
Thanks for adding the comment here: 
Note that the selection is based on Ethernet connection (install) and toggle
developer switch to consumer mode (reset)

[Che-Liang Chiou, 2011-03-30 09:54:38]

Updated. PTAL.

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh
File factory_install.sh (right):

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh#newcode74
factory_install.sh:74: local candidates=$(ifconfig | grep 'Link encap:Ethernet'
| cut -d ' ' -f 1)
On 2011/03/30 09:17:53, Hung-Te wrote:
> Please quote the $() to "$()", if you use "local".

Done. Just for curious: why local needs quote? (does globals not?)

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh#newcode77
factory_install.sh:77: if ! iw $candidate info > /dev/null 2>&1; then
On 2011/03/30 09:17:53, Hung-Te wrote:
> >/dev/null, no need for the space.

Done.

[Hung-Te, 2011-03-30 10:34:40]

LGTM, assuming the latest version still works correctly.

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh
File factory_install.sh (right):

http://codereview.chromium.org/6708106/diff/9001/factory_install.sh#newcode74
factory_install.sh:74: local candidates=$(ifconfig | grep 'Link encap:Ethernet'
| cut -d ' ' -f 1)
On 2011/03/30 09:54:38, Che-Liang Chiou wrote:
> Done. Just for curious: why local needs quote? (does globals not?)
 bash and dash interpret "local" and $() in different way.
 if ls gives you "a", "b", "c"
 local a=$(ls)
 bash: local a="a b c"; echo $a => a b c
 dash: local a=a b c; echo $a => a