Initial version of tpm_init, a library for taking ownership of the TPM.

Initial version of tpm_init, a library for taking ownership of the TPM.

BUG=3065, 4657
TEST=manual

[Chris Masone, 2010-07-29 20:58:00]

mostly small stuff

http://codereview.chromium.org/3048029/diff/3001/4004
File main.cc (right):

http://codereview.chromium.org/3048029/diff/3001/4004#newcode41
main.cc:41: std::cout << "TPM Onwer Password: " << str_password << std::endl;
s/Onwer/Owner/

http://codereview.chromium.org/3048029/diff/3001/4007
File tpm.cc (right):

http://codereview.chromium.org/3048029/diff/3001/4007#newcode42
tpm.cc:42: is_owned_ = IsOwnedCheckViaSysfs();
this might block, if ownership is currently being taken.  Note with a comment,
at least?

http://codereview.chromium.org/3048029/diff/3001/4007#newcode114
tpm.cc:114: PlatformThread::Sleep(100);
maybe a constant?  kTpmRetryIntervalMs?

http://codereview.chromium.org/3048029/diff/3001/4007#newcode448
tpm.cc:448: const_cast<BYTE *>(static_cast<const BYTE *>(
I love using C APIs from C++ and the attendant casting, don't you?

http://codereview.chromium.org/3048029/diff/3001/4007#newcode460
tpm.cc:460: if (const_cast<Tpm*>(this)->sync_lock_.Try()) {
???why do you have to do these weird gymnastics?

sync_lock_.Try() doesn't work?

http://codereview.chromium.org/3048029/diff/3001/4008
File tpm.h (right):

http://codereview.chromium.org/3048029/diff/3001/4008#newcode67
tpm.h:67: //bool Activate(TSS_HCONTEXT context_handle);
Can these be taken out?  We can't do them on our platform, right?  Or maybe they
should be in for completeness but fail with a CHECK() or NOT_IMPLEMENTED()

http://codereview.chromium.org/3048029/diff/3001/4008#newcode95
tpm.h:95: Lock sync_lock_;
might be nice to know what data this Lock is meant to protect, and against what.

[rginda, 2010-07-30 18:49:50]

LGTM with two comments.

http://codereview.chromium.org/3048029/diff/1/5
File main.cc (right):

http://codereview.chromium.org/3048029/diff/1/5#newcode24
main.cc:24: logging::InitLogging(NULL,
LOG_ONLY_TO_SYSTEM_DEBUG_LOG logs to a chrome-specific log file, which I imagine
will be overwritten when chrome starts.

http://codereview.chromium.org/3048029/diff/1/6
File secure_blob.cc (right):

http://codereview.chromium.org/3048029/diff/1/6#newcode2
secure_blob.cc:2: // Use of this source code is governed by a BSD-style license
that can be
This would probably be useful to have in common/chromeos/... eventually.

[fes, 2010-08-02 17:51:10]

Addressed feedback.

http://codereview.chromium.org/3048029/diff/1/5
File main.cc (right):

http://codereview.chromium.org/3048029/diff/1/5#newcode24
main.cc:24: logging::InitLogging(NULL,
On 2010/07/30 18:49:50, rginda wrote:
> LOG_ONLY_TO_SYSTEM_DEBUG_LOG logs to a chrome-specific log file, which I
imagine
> will be overwritten when chrome starts.
> 

Done.

http://codereview.chromium.org/3048029/diff/1/6
File secure_blob.cc (right):

http://codereview.chromium.org/3048029/diff/1/6#newcode2
secure_blob.cc:2: // Use of this source code is governed by a BSD-style license
that can be
Agreed, eventually.  Right now there is duplication between c-home and tpm_init
until it is added to libchromeos.

http://codereview.chromium.org/3048029/diff/3001/4004
File main.cc (right):

http://codereview.chromium.org/3048029/diff/3001/4004#newcode41
main.cc:41: std::cout << "TPM Onwer Password: " << str_password << std::endl;
On 2010/07/29 20:58:00, cmasone wrote:
> s/Onwer/Owner/

Done.

http://codereview.chromium.org/3048029/diff/3001/4007
File tpm.cc (right):

http://codereview.chromium.org/3048029/diff/3001/4007#newcode42
tpm.cc:42: is_owned_ = IsOwnedCheckViaSysfs();
Added a comment explaining this, and why the call to IsOwned() uses the cached
result from Init() instead of re-checking.  Since c-home will be the process
that takes ownership of the TPM, this shouldn't present a problem, but it is
certainly worth noting in case things change.

http://codereview.chromium.org/3048029/diff/3001/4007#newcode114
tpm.cc:114: PlatformThread::Sleep(100);
On 2010/07/29 20:58:00, cmasone wrote:
> maybe a constant?  kTpmRetryIntervalMs?

Done.

http://codereview.chromium.org/3048029/diff/3001/4007#newcode448
tpm.cc:448: const_cast<BYTE *>(static_cast<const BYTE *>(
Yes.  Arguably these calls for class Tpm shouldn't be const anyway, so I've
removed that, which allows removing the casting.

http://codereview.chromium.org/3048029/diff/3001/4007#newcode460
tpm.cc:460: if (const_cast<Tpm*>(this)->sync_lock_.Try()) {
It does now that I removed const from GetOwnerPassword().

http://codereview.chromium.org/3048029/diff/3001/4008
File tpm.h (right):

http://codereview.chromium.org/3048029/diff/3001/4008#newcode67
tpm.h:67: //bool Activate(TSS_HCONTEXT context_handle);
On 2010/07/29 20:58:00, cmasone wrote:
> Can these be taken out?  We can't do them on our platform, right?  Or maybe
they
> should be in for completeness but fail with a CHECK() or NOT_IMPLEMENTED()

Done.

http://codereview.chromium.org/3048029/diff/3001/4008#newcode95
tpm.h:95: Lock sync_lock_;
On 2010/07/29 20:58:00, cmasone wrote:
> might be nice to know what data this Lock is meant to protect, and against
what.

Done.

[Chris Masone, 2010-08-02 17:58:05]

lgtm