Additional cases for the test suite, and more commands added to TLCL

Additional cases for the test suite, and more commands added to TLCL
to support the new cases.

[Luigi Semenzato, 2010-03-16 21:47:02]

Current state of testing.

[gauravsh, 2010-03-17 00:15:24]

My high level comment is that you should clarify what the test expectations are
- and inform the caller (which will eventually be a driver program) whether the
test succeeded or failed.

http://codereview.chromium.org/1056001/diff/2001/3003
File src/platform/tpm_lite/src/testsuite/clear.c (right):

http://codereview.chromium.org/1056001/diff/2001/3003#newcode38
src/platform/tpm_lite/src/testsuite/clear.c:38: printf("tpm is %sowned\n",
owned?  "" : "NOT ");
Nit: space between %s and owned

http://codereview.chromium.org/1056001/diff/2001/3003#newcode43
src/platform/tpm_lite/src/testsuite/clear.c:43: 
If these are tests, how do they inform the caller (environment) whether they
failed or succeeded?

http://codereview.chromium.org/1056001/diff/2001/3004
File src/platform/tpm_lite/src/testsuite/lock.c (right):

http://codereview.chromium.org/1056001/diff/2001/3004#newcode30
src/platform/tpm_lite/src/testsuite/lock.c:30: exit(0);
same comment as clear.c: How is success or failure determined/returned?

http://codereview.chromium.org/1056001/diff/2001/3005
File src/platform/tpm_lite/src/testsuite/writelimit.c (right):

http://codereview.chromium.org/1056001/diff/2001/3005#newcode17
src/platform/tpm_lite/src/testsuite/writelimit.c:17: 
Nit: vertical whitespace

http://codereview.chromium.org/1056001/diff/2001/3005#newcode22
src/platform/tpm_lite/src/testsuite/writelimit.c:22: uint8_t disable,
deactivated;
s/disable/disabled/?

http://codereview.chromium.org/1056001/diff/2001/3005#newcode41
src/platform/tpm_lite/src/testsuite/writelimit.c:41: for (i = 0; i < 66; i++) {
make this a constant macro, instead of hard coding a value. 

Add a comment about where the value is derived from.

http://codereview.chromium.org/1056001/diff/2001/3005#newcode52
src/platform/tpm_lite/src/testsuite/writelimit.c:52: exit(0);
Again, this is a test failure. This needs to be notified.

[Luigi Semenzato, 2010-03-17 05:08:28]

Thanks Gaurav.

http://codereview.chromium.org/1056001/diff/2001/3003
File src/platform/tpm_lite/src/testsuite/clear.c (right):

http://codereview.chromium.org/1056001/diff/2001/3003#newcode38
src/platform/tpm_lite/src/testsuite/clear.c:38: printf("tpm is %sowned\n",
owned?  "" : "NOT ");
On 2010/03/17 00:15:24, gauravsh wrote:
> Nit: space between %s and owned

The space is the one after the "NOT".  Otherwise I get two spaces.

http://codereview.chromium.org/1056001/diff/2001/3003#newcode43
src/platform/tpm_lite/src/testsuite/clear.c:43: 
On 2010/03/17 00:15:24, gauravsh wrote:
> If these are tests, how do they inform the caller (environment) whether they
> failed or succeeded?

Yes, you're right---the tests should be scriptable.  I used this particular test
mostly to check my implementation of IsOwned and ForceClear, since there already
is a tpm_clear program in tpm_tools.  Those commands are implemented so that if
they emit warnings for failures, since at this stage I was more interested in a
visual verification of the output.  But I agree that I need a driver script and
better error reporting conventions.  Should I do both in a following change or
in this one?

Actually, I can't run these tests automatically until I have a machine that can
reboot without hanging...

http://codereview.chromium.org/1056001/diff/2001/3005
File src/platform/tpm_lite/src/testsuite/writelimit.c (right):

http://codereview.chromium.org/1056001/diff/2001/3005#newcode22
src/platform/tpm_lite/src/testsuite/writelimit.c:22: uint8_t disable,
deactivated;
On 2010/03/17 00:15:24, gauravsh wrote:
> s/disable/disabled/?

It would look better, wouldn't it?  I picked those names because that's what
those flags are called in the TPM docs and I'd rather follow their bad naming
convention than change it, because there may be some subtle nuance in meaning. 
But I'll put a comment in there.

http://codereview.chromium.org/1056001/diff/2001/3005#newcode41
src/platform/tpm_lite/src/testsuite/writelimit.c:41: for (i = 0; i < 66; i++) {
On 2010/03/17 00:15:24, gauravsh wrote:
> make this a constant macro, instead of hard coding a value. 
> 
> Add a comment about where the value is derived from.

Yes, thanks.

http://codereview.chromium.org/1056001/diff/2001/3005#newcode52
src/platform/tpm_lite/src/testsuite/writelimit.c:52: exit(0);
On 2010/03/17 00:15:24, gauravsh wrote:
> Again, this is a test failure. This needs to be notified.

In fact, error() calls exit(1)---I'll make that clear thanks.

[gauravsh, 2010-03-17 18:56:28]

lgtm

http://codereview.chromium.org/1056001/diff/2001/3003
File src/platform/tpm_lite/src/testsuite/clear.c (right):

http://codereview.chromium.org/1056001/diff/2001/3003#newcode38
src/platform/tpm_lite/src/testsuite/clear.c:38: printf("tpm is %sowned\n",
owned?  "" : "NOT ");
On 2010/03/17 05:08:28, Luigi Semenzato wrote:
> On 2010/03/17 00:15:24, gauravsh wrote:
> > Nit: space between %s and owned
> 
> The space is the one after the "NOT".  Otherwise I get two spaces.

Ok. I missed that.

http://codereview.chromium.org/1056001/diff/2001/3003#newcode43
src/platform/tpm_lite/src/testsuite/clear.c:43: 
On 2010/03/17 05:08:28, Luigi Semenzato wrote:
> On 2010/03/17 00:15:24, gauravsh wrote:
> > If these are tests, how do they inform the caller (environment) whether they
> > failed or succeeded?
> 
> Yes, you're right---the tests should be scriptable.  I used this particular
test
> mostly to check my implementation of IsOwned and ForceClear, since there
already
> is a tpm_clear program in tpm_tools.  Those commands are implemented so that
if
> they emit warnings for failures, since at this stage I was more interested in
a
> visual verification of the output.  But I agree that I need a driver script
and
> better error reporting conventions.  Should I do both in a following change or
> in this one?
> 
> Actually, I can't run these tests automatically until I have a machine that
can
> reboot without hanging...

You can make the change in a separate CL. Let's get these in for now.