Fixed a minor 'umask' that couldn't have worked.

Removed a broken 'umask' call.

This probably caused the tcsd call to spawn twice in some cases.

BUG=chromium-os:13510
TEST=Manually typed the following commands in a shell and validated that they produced no errors.

umask
current_mask=$(umask)
umask ${current_mask}
umask
set | grep current_mask

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

[diandersAtChromium, 2011-04-04 23:50:15]

Richard,

A tiny cleanup I noticed during code inspection.  I don't think it matters a
whole lot, but it's nice to not have a syntax error in our shell scripts.

I have some other minor cleanups/speedups to this script for cleanups, but since
it looks pretty hard to test I probably won't submit them unless you're
interested.

Thanks!

http://codereview.chromium.org/6793028/diff/1/tcsd.conf
File tcsd.conf (right):

http://codereview.chromium.org/6793028/diff/1/tcsd.conf#newcode18
tcsd.conf:18: # Temporary fix for BIOS bug.  Very Soon[tm] this fix will be in
the BIOS.
I have asked Luigi if this is still needed.

[jrbarnette, 2011-04-05 00:52:38]

This change might actually affect boot time, although maybe
not much...

http://codereview.chromium.org/6793028/diff/1/tcsd.conf
File tcsd.conf (right):

http://codereview.chromium.org/6793028/diff/1/tcsd.conf#newcode26
tcsd.conf:26: binf1=/sys/devices/platform/chromeos_acpi/BINF.1
There should be a bug filed for this obsolete usage of chromeos_acpi;
the preferred idiom for this access is a new command called
cross_system.

http://codereview.chromium.org/6793028/diff/1/tcsd.conf#newcode58
tcsd.conf:58: umask ${current_mask}
Yeesh.  Yes, it was broken.
The impact was more than a syntax error: the failure would
have been something along the lines of "command not found".
This script runs "set -e", so it would have terminated the script
with a non-zero status here, if it executed.

I'm surprised tcsd even starts.  I suspect it works because
upstart respawns the 'pre-start' script, and the broken code
path doesn't execute the second time...

As for the fix:  restoring umask here is pointless; umask is
private to the process, and the process dies when the script
terminates.  I'd recommend deleting this line, rather than
fixing it.

[diandersAtChromium, 2011-04-05 03:10:37]

OK, umask restoring (and saving) is removed.  Bugs filed for other issues. 
PTAL.

I would imagine that the bit of code with the bug might not be exercised a whole
lot, since it's only in the case that the device is owned and has zero-sized
system data.  Is that common?  I don't seem to see any errors in my
/var/log/messages about it.

http://codereview.chromium.org/6793028/diff/1/tcsd.conf
File tcsd.conf (right):

http://codereview.chromium.org/6793028/diff/1/tcsd.conf#newcode18
tcsd.conf:18: # Temporary fix for BIOS bug.  Very Soon[tm] this fix will be in
the BIOS.
FYI: this is fixed as <http://crosbug.com/13819>

On 2011/04/04 23:50:15, diandersAtChromium wrote:
> I have asked Luigi if this is still needed.

http://codereview.chromium.org/6793028/diff/1/tcsd.conf#newcode26
tcsd.conf:26: binf1=/sys/devices/platform/chromeos_acpi/BINF.1
Done.  <http://crosbug.com/13833>.  Do you want to be the owner, or should I
leave it ownerless for now?

On 2011/04/05 00:52:38, jrbarnette wrote:
> There should be a bug filed for this obsolete usage of chromeos_acpi;
> the preferred idiom for this access is a new command called
> cross_system.

http://codereview.chromium.org/6793028/diff/1/tcsd.conf#newcode58
tcsd.conf:58: umask ${current_mask}
On 2011/04/05 00:52:38, jrbarnette wrote:
> Yeesh.  Yes, it was broken.
> The impact was more than a syntax error: the failure would
> have been something along the lines of "command not found".
> This script runs "set -e", so it would have terminated the script
> with a non-zero status here, if it executed.
> 
> I'm surprised tcsd even starts.  I suspect it works because
> upstart respawns the 'pre-start' script, and the broken code
> path doesn't execute the second time...
> 
> As for the fix:  restoring umask here is pointless; umask is
> private to the process, and the process dies when the script
> terminates.  I'd recommend deleting this line, rather than
> fixing it.

Done.

[jrbarnette, 2011-04-05 16:32:05]

On 2011/04/05 03:10:37, diandersAtChromium wrote:
> OK, umask restoring (and saving) is removed.  Bugs filed for other issues. 
> PTAL.
> 
> I would imagine that the bit of code with the bug might not be exercised a
whole
> lot, since it's only in the case that the device is owned and has zero-sized
> system data.  Is that common?  I don't seem to see any errors in my
> /var/log/messages about it.
> 
The only thing the job is expected to send to /var/log/messages
is this message:  "tpm fix status: $status"

The code with the bug exists to create /var/lib/tpm/system.data,
and won't run unless that file is missing.  Since the broken 'umask'
is the last command in the script, I think it creates this sequence
  * The initial parts of the job run normally, /var/lib/tpm/system.data
    gets created and then the broken umask runs (the latter can't happen
    without the former).
  * The broken umask causes the script to terminate with a non-zero
    status.
  * /sbin/init sees the bad status and respawns the job.
  * The pre-start script runs again, and since the 'system.data' file
    exists this time, the broken umask doesn't execute and the
    job proceeds normally.

/var/lib/tpm/system.data will persist across reboots and updates,
so this failure path is probably rare in practice.  Still, it's wrong, and
this change fixes it.

LGTM and thanks!

> http://codereview.chromium.org/6793028/diff/1/tcsd.conf
> File tcsd.conf (right):
> 
> http://codereview.chromium.org/6793028/diff/1/tcsd.conf#newcode18
> tcsd.conf:18: # Temporary fix for BIOS bug.  Very Soon[tm] this fix will be in
> the BIOS.
> FYI: this is fixed as <http://crosbug.com/13819>
> 
> On 2011/04/04 23:50:15, diandersAtChromium wrote:
> > I have asked Luigi if this is still needed.
> 
> http://codereview.chromium.org/6793028/diff/1/tcsd.conf#newcode26
> tcsd.conf:26: binf1=/sys/devices/platform/chromeos_acpi/BINF.1
> Done.  <http://crosbug.com/13833>.  Do you want to be the owner, or should I
> leave it ownerless for now?
> 
> On 2011/04/05 00:52:38, jrbarnette wrote:
> > There should be a bug filed for this obsolete usage of chromeos_acpi;
> > the preferred idiom for this access is a new command called
> > cross_system.
> 
> http://codereview.chromium.org/6793028/diff/1/tcsd.conf#newcode58
> tcsd.conf:58: umask ${current_mask}
> On 2011/04/05 00:52:38, jrbarnette wrote:
> > Yeesh.  Yes, it was broken.
> > The impact was more than a syntax error: the failure would
> > have been something along the lines of "command not found".
> > This script runs "set -e", so it would have terminated the script
> > with a non-zero status here, if it executed.
> > 
> > I'm surprised tcsd even starts.  I suspect it works because
> > upstart respawns the 'pre-start' script, and the broken code
> > path doesn't execute the second time...
> > 
> > As for the fix:  restoring umask here is pointless; umask is
> > private to the process, and the process dies when the script
> > terminates.  I'd recommend deleting this line, rather than
> > fixing it.
> 
> Done.

[jrbarnette, 2011-04-05 16:37:01]

I should add one more thing:

To fully test that this code change has executed, and not failed
utterly, you should do this:
  * Install from USB and reboot
  * From shell prompt, confirm that tcsd is executing (e.g.
     'ps -C tcsd').
This won't prove that the bug is fixed, but it _will_ show you
haven't made anything any worse.