Update on feedback, update dbus API, add unit tests. TEST=manual,unit,BVT BUG=3628 323

Update on feedback, update dbus API, add unit tests. TEST=manual,unit,BVT BUG=3628
323

This CL includes changes based on code review feedback, an updated dbus API which adds MountGuest, and refactors code to allow for unit testing.  Functionally, it is compatible with prior versions of cryptohome.

[Will Drewry, 2010-06-09 19:08:12]

Looks good but there are still some style nits hanging around :/ After realizing
I wasn't remembering all the style rules so well, I started linking to the right
stanzas.  Please scan all the feedback to make sure I didn't contradict myself
too badly.

Thanks!
will

http://codereview.chromium.org/2645008/diff/1/4
File crypto.cc (right):

http://codereview.chromium.org/2645008/diff/1/4#newcode5
crypto.cc:5: // Contains the implementation of class Mount
Crypto (not Mount)

http://codereview.chromium.org/2645008/diff/1/4#newcode195
crypto.cc:195: int holder_size = (update_length > SHA_DIGEST_LENGTH
style: Usually multiline conditionals should be if() {}.  

E.g.,
int holder_size = SHA_DIGEST_LENGTH;
if (update_length > SHA_DIGEST_LENGTH)
  holder_size = update_length;

http://codereview.chromium.org/2645008/diff/1/4#newcode265
crypto.cc:265: *key_signature = chromeos::AsciiEncode(vault_keyset.FEK_SIG());
Did you notice that AsciiEncode can just take down the process in DecodeChar? 
:/  I don't think you'll hit that case though.

http://codereview.chromium.org/2645008/diff/1/5
File crypto.h (right):

http://codereview.chromium.org/2645008/diff/1/5#newcode10
crypto.h:10: #ifndef CRYPTO_H_
CRYPTOHOME_CRYPTO_H_?

http://codereview.chromium.org/2645008/diff/1/5#newcode52
crypto.h:52: // Wraps (encrpyts) the vault keyset with the given wrapper
typo

http://codereview.chromium.org/2645008/diff/1/5#newcode61
crypto.h:61: const SecureBlob& vault_wrapper,
is there some weird tabs v spaces thing going on here or are these arguments and
hte arguments for the next prototype aligned wrong?

http://codereview.chromium.org/2645008/diff/1/5#newcode138
crypto.h:138: }
// namespace cryptohome

http://codereview.chromium.org/2645008/diff/1/6
File crypto_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/1/6#newcode46
crypto_unittest.cc:46: protected:
style: you can probably just leave out the empty sections
but under private you could still stop COPY_AND_ASSIGN.

http://codereview.chromium.org/2645008/diff/1/6#newcode127
crypto_unittest.cc:127: FilePath
salt_path(FilePath(kImageDir).Append("crypto_test_salt"));
does FilePath or file_util support mocking out the filesystem?

http://codereview.chromium.org/2645008/diff/1/7
File cryptohome.cc (right):

http://codereview.chromium.org/2645008/diff/1/7#newcode86
cryptohome.cc:86: std::string* old_password_out = NULL) {
style: this is still wrong :/  Default values aren't really allowed style-wise
afaik.  (Maybe I'm just wrong :)  Callers should either explicitly pass in NULL
or there should be two functions:
GetUsernamePassword(...) -> calls ->
GetUsernamePasswords(..., old_password_out)

That would keep the callsites intact and let a migrate call path just call the
underlying function.

A TODO would be nice for this cl - but since I don't see any callers using it,
it might be an easy enough change to pull in.

http://codereview.chromium.org/2645008/diff/1/14
File make_tests.sh (right):

http://codereview.chromium.org/2645008/diff/1/14#newcode2
make_tests.sh:2: # Copyright (c) 2009 The Chromium OS Authors. All rights
reserved.
nit: 2010

http://codereview.chromium.org/2645008/diff/1/15
File mount.cc (right):

http://codereview.chromium.org/2645008/diff/1/15#newcode61
mount.cc:61: if (crypto_)
Do you want to delete crypto_ and platform_ even if they were passed in by an
external caller?  It means it can't be used like:
MockPlatform platform;
MockCrypto crypto;
Mount m(..., &crypto, &platform);

If you go with the default_impl_ scoped_ptr, then you can avoid having to take
ownership of the pointer or track if you did.  

If you want it taking ownership, please explicitly state that it does in the
comment on Mount() in mount.h

http://codereview.chromium.org/2645008/diff/1/16
File mount.h (right):

http://codereview.chromium.org/2645008/diff/1/16#newcode291
mount.h:291: Crypto* crypto_;
scoped_ptr<Crypto> crypto_;

Or since you want to optionally pass in a pointer.  If you don't want to take
ownership:
scoped_ptr<Crypto> default_crypto_;
Crypto *crypto_;

and have crypto_ = default_crypto_.get() unless it was passed in.

http://codereview.chromium.org/2645008/diff/1/16#newcode294
mount.h:294: Platform* platform_;
ditto

http://codereview.chromium.org/2645008/diff/1/18
File platform.cc (right):

http://codereview.chromium.org/2645008/diff/1/18#newcode290
platform.cc:290: } // cryptohome
// namespace cryptohome

http://codereview.chromium.org/2645008/diff/1/19
File platform.h (right):

http://codereview.chromium.org/2645008/diff/1/19#newcode5
platform.h:5: #ifndef PLATFORM_H_
CRYPTOHOME_PLATFORM_H_?

http://codereview.chromium.org/2645008/diff/1/19#newcode32
platform.h:32: Platform(int mount_options, int umask, const std::string&
mtab_file,
hrm so I was wrong about the cryptohome.cc thing:
 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

But overloading here, isn't cool: 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

To support injectable dependencies or non-default configs, I usually use setters
which should be called before any other function is used and note that in the
comment.  If there is an Init() function, then I'll make that set a bool and
have those setters be required prior to init (if appropriate).

Another options is to have the constructor call some default_setter if
Plaform(bool use_defaults).  then if not, they need to be set.  I'm sure there
are better ways too.

http://codereview.chromium.org/2645008/diff/1/19#newcode104
platform.h:104: }
// namespace ...

http://codereview.chromium.org/2645008/diff/1/22
File secure_blob_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/1/22#newcode14
secure_blob_unittest.cc:14: using namespace chromeos;
style: pulling in a whole namespace is generally not ok.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

http://codereview.chromium.org/2645008/diff/1/22#newcode18
secure_blob_unittest.cc:18: void SetUp() {
Should this be before public:?

http://codereview.chromium.org/2645008/diff/1/22#newcode38
secure_blob_unittest.cc:38: 
nit: empty sections not really needed.

http://codereview.chromium.org/2645008/diff/1/29
File username_passkey.h (right):

http://codereview.chromium.org/2645008/diff/1/29#newcode24
username_passkey.h:24: const chromeos::Blob& salt);
style: function overloading should be avoided :/

http://codereview.chromium.org/2645008/diff/1/31
File vault_keyset.cc (right):

http://codereview.chromium.org/2645008/diff/1/31#newcode73
vault_keyset.cc:73: Min(fek_sig_.size(), sizeof(keys.fek_sig)));
size() and sizeof() aren't signed 32-bit ints but vector::size_type and size_t
and memcpy expects a size_t.  If your Min function is explicitly only for these
cases, then it would make more sense to have it compare size_t which would avoid
potential truncation if we go 64-bit and any other casting weirdness.

Noot that those are likely in any of these scenarios :), but it'd just be
cleaner to not force a type conversion when you know in advance you don't need
one.

std::min might also work for you -- it is just a templated min test.  It is
subject to some weirdness but should work fine when comparing to size_ts.

http://codereview.chromium.org/2645008/diff/1/33
File vault_keyset_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/1/33#newcode16
vault_keyset_unittest.cc:16: using namespace chromeos;
style: dont pull in a full namespace; alias any needed names

http://codereview.chromium.org/2645008/diff/1/33#newcode20
vault_keyset_unittest.cc:20: void SetUp() {
nit: outside of declared section

http://codereview.chromium.org/2645008/diff/1/33#newcode39
vault_keyset_unittest.cc:39: protected:
nit: protected and private not really needed.

[fes, 2010-06-09 21:24:59]

Great feedback--sorry for all of the style violations.  These should be fixed in
the latest diff (along with the same issues I found in other locations). 
Running BVT shortly.

fes

On 2010/06/09 19:08:12, Will Drewry wrote:
> Looks good but there are still some style nits hanging around :/ After
realizing
> I wasn't remembering all the style rules so well, I started linking to the
right
> stanzas.  Please scan all the feedback to make sure I didn't contradict myself
> too badly.
> 
> Thanks!
> will
> 
> http://codereview.chromium.org/2645008/diff/1/4
> File crypto.cc (right):
> 
> http://codereview.chromium.org/2645008/diff/1/4#newcode5
> crypto.cc:5: // Contains the implementation of class Mount
> Crypto (not Mount)
> 
> http://codereview.chromium.org/2645008/diff/1/4#newcode195
> crypto.cc:195: int holder_size = (update_length > SHA_DIGEST_LENGTH
> style: Usually multiline conditionals should be if() {}.  
> 
> E.g.,
> int holder_size = SHA_DIGEST_LENGTH;
> if (update_length > SHA_DIGEST_LENGTH)
>   holder_size = update_length;
> 
> http://codereview.chromium.org/2645008/diff/1/4#newcode265
> crypto.cc:265: *key_signature = chromeos::AsciiEncode(vault_keyset.FEK_SIG());
> Did you notice that AsciiEncode can just take down the process in DecodeChar? 
> :/  I don't think you'll hit that case though.
> 
> http://codereview.chromium.org/2645008/diff/1/5
> File crypto.h (right):
> 
> http://codereview.chromium.org/2645008/diff/1/5#newcode10
> crypto.h:10: #ifndef CRYPTO_H_
> CRYPTOHOME_CRYPTO_H_?
> 
> http://codereview.chromium.org/2645008/diff/1/5#newcode52
> crypto.h:52: // Wraps (encrpyts) the vault keyset with the given wrapper
> typo
> 
> http://codereview.chromium.org/2645008/diff/1/5#newcode61
> crypto.h:61: const SecureBlob& vault_wrapper,
> is there some weird tabs v spaces thing going on here or are these arguments
and
> hte arguments for the next prototype aligned wrong?
> 
> http://codereview.chromium.org/2645008/diff/1/5#newcode138
> crypto.h:138: }
> // namespace cryptohome
> 
> http://codereview.chromium.org/2645008/diff/1/6
> File crypto_unittest.cc (right):
> 
> http://codereview.chromium.org/2645008/diff/1/6#newcode46
> crypto_unittest.cc:46: protected:
> style: you can probably just leave out the empty sections
> but under private you could still stop COPY_AND_ASSIGN.
> 
> http://codereview.chromium.org/2645008/diff/1/6#newcode127
> crypto_unittest.cc:127: FilePath
> salt_path(FilePath(kImageDir).Append("crypto_test_salt"));
> does FilePath or file_util support mocking out the filesystem?
> 
> http://codereview.chromium.org/2645008/diff/1/7
> File cryptohome.cc (right):
> 
> http://codereview.chromium.org/2645008/diff/1/7#newcode86
> cryptohome.cc:86: std::string* old_password_out = NULL) {
> style: this is still wrong :/  Default values aren't really allowed style-wise
> afaik.  (Maybe I'm just wrong :)  Callers should either explicitly pass in
NULL
> or there should be two functions:
> GetUsernamePassword(...) -> calls ->
> GetUsernamePasswords(..., old_password_out)
> 
> That would keep the callsites intact and let a migrate call path just call the
> underlying function.
> 
> A TODO would be nice for this cl - but since I don't see any callers using it,
> it might be an easy enough change to pull in.
> 
> http://codereview.chromium.org/2645008/diff/1/14
> File make_tests.sh (right):
> 
> http://codereview.chromium.org/2645008/diff/1/14#newcode2
> make_tests.sh:2: # Copyright (c) 2009 The Chromium OS Authors. All rights
> reserved.
> nit: 2010
> 
> http://codereview.chromium.org/2645008/diff/1/15
> File mount.cc (right):
> 
> http://codereview.chromium.org/2645008/diff/1/15#newcode61
> mount.cc:61: if (crypto_)
> Do you want to delete crypto_ and platform_ even if they were passed in by an
> external caller?  It means it can't be used like:
> MockPlatform platform;
> MockCrypto crypto;
> Mount m(..., &crypto, &platform);
> 
> If you go with the default_impl_ scoped_ptr, then you can avoid having to take
> ownership of the pointer or track if you did.  
> 
> If you want it taking ownership, please explicitly state that it does in the
> comment on Mount() in mount.h
> 
> http://codereview.chromium.org/2645008/diff/1/16
> File mount.h (right):
> 
> http://codereview.chromium.org/2645008/diff/1/16#newcode291
> mount.h:291: Crypto* crypto_;
> scoped_ptr<Crypto> crypto_;
> 
> Or since you want to optionally pass in a pointer.  If you don't want to take
> ownership:
> scoped_ptr<Crypto> default_crypto_;
> Crypto *crypto_;
> 
> and have crypto_ = default_crypto_.get() unless it was passed in.
> 
> http://codereview.chromium.org/2645008/diff/1/16#newcode294
> mount.h:294: Platform* platform_;
> ditto
> 
> http://codereview.chromium.org/2645008/diff/1/18
> File platform.cc (right):
> 
> http://codereview.chromium.org/2645008/diff/1/18#newcode290
> platform.cc:290: } // cryptohome
> // namespace cryptohome
> 
> http://codereview.chromium.org/2645008/diff/1/19
> File platform.h (right):
> 
> http://codereview.chromium.org/2645008/diff/1/19#newcode5
> platform.h:5: #ifndef PLATFORM_H_
> CRYPTOHOME_PLATFORM_H_?
> 
> http://codereview.chromium.org/2645008/diff/1/19#newcode32
> platform.h:32: Platform(int mount_options, int umask, const std::string&
> mtab_file,
> hrm so I was wrong about the cryptohome.cc thing:
>  
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...
> 
> But overloading here, isn't cool: 
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> To support injectable dependencies or non-default configs, I usually use
setters
> which should be called before any other function is used and note that in the
> comment.  If there is an Init() function, then I'll make that set a bool and
> have those setters be required prior to init (if appropriate).
> 
> Another options is to have the constructor call some default_setter if
> Plaform(bool use_defaults).  then if not, they need to be set.  I'm sure there
> are better ways too.
> 
> http://codereview.chromium.org/2645008/diff/1/19#newcode104
> platform.h:104: }
> // namespace ...
> 
> http://codereview.chromium.org/2645008/diff/1/22
> File secure_blob_unittest.cc (right):
> 
> http://codereview.chromium.org/2645008/diff/1/22#newcode14
> secure_blob_unittest.cc:14: using namespace chromeos;
> style: pulling in a whole namespace is generally not ok.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
> 
> http://codereview.chromium.org/2645008/diff/1/22#newcode18
> secure_blob_unittest.cc:18: void SetUp() {
> Should this be before public:?
> 
> http://codereview.chromium.org/2645008/diff/1/22#newcode38
> secure_blob_unittest.cc:38: 
> nit: empty sections not really needed.
> 
> http://codereview.chromium.org/2645008/diff/1/29
> File username_passkey.h (right):
> 
> http://codereview.chromium.org/2645008/diff/1/29#newcode24
> username_passkey.h:24: const chromeos::Blob& salt);
> style: function overloading should be avoided :/
> 
> http://codereview.chromium.org/2645008/diff/1/31
> File vault_keyset.cc (right):
> 
> http://codereview.chromium.org/2645008/diff/1/31#newcode73
> vault_keyset.cc:73: Min(fek_sig_.size(), sizeof(keys.fek_sig)));
> size() and sizeof() aren't signed 32-bit ints but vector::size_type and size_t
> and memcpy expects a size_t.  If your Min function is explicitly only for
these
> cases, then it would make more sense to have it compare size_t which would
avoid
> potential truncation if we go 64-bit and any other casting weirdness.
> 
> Noot that those are likely in any of these scenarios :), but it'd just be
> cleaner to not force a type conversion when you know in advance you don't need
> one.
> 
> std::min might also work for you -- it is just a templated min test.  It is
> subject to some weirdness but should work fine when comparing to size_ts.
> 
> http://codereview.chromium.org/2645008/diff/1/33
> File vault_keyset_unittest.cc (right):
> 
> http://codereview.chromium.org/2645008/diff/1/33#newcode16
> vault_keyset_unittest.cc:16: using namespace chromeos;
> style: dont pull in a full namespace; alias any needed names
> 
> http://codereview.chromium.org/2645008/diff/1/33#newcode20
> vault_keyset_unittest.cc:20: void SetUp() {
> nit: outside of declared section
> 
> http://codereview.chromium.org/2645008/diff/1/33#newcode39
> vault_keyset_unittest.cc:39: protected:
> nit: protected and private not really needed.

[fes, 2010-06-09 21:25:25]

http://codereview.chromium.org/2645008/diff/1/4
File crypto.cc (right):

http://codereview.chromium.org/2645008/diff/1/4#newcode5
crypto.cc:5: // Contains the implementation of class Mount
On 2010/06/09 19:08:12, Will Drewry wrote:
> Crypto (not Mount)

Done.

http://codereview.chromium.org/2645008/diff/1/4#newcode195
crypto.cc:195: int holder_size = (update_length > SHA_DIGEST_LENGTH
On 2010/06/09 19:08:12, Will Drewry wrote:
> style: Usually multiline conditionals should be if() {}.  
> 
> E.g.,
> int holder_size = SHA_DIGEST_LENGTH;
> if (update_length > SHA_DIGEST_LENGTH)
>   holder_size = update_length;
> 

Done.

http://codereview.chromium.org/2645008/diff/1/4#newcode265
crypto.cc:265: *key_signature = chromeos::AsciiEncode(vault_keyset.FEK_SIG());
On 2010/06/09 19:08:12, Will Drewry wrote:
> Did you notice that AsciiEncode can just take down the process in DecodeChar? 
> :/  I don't think you'll hit that case though.

AsciiDecode, right?  I only use that in a unit test.

http://codereview.chromium.org/2645008/diff/1/5
File crypto.h (right):

http://codereview.chromium.org/2645008/diff/1/5#newcode10
crypto.h:10: #ifndef CRYPTO_H_
On 2010/06/09 19:08:12, Will Drewry wrote:
> CRYPTOHOME_CRYPTO_H_?

Done.

http://codereview.chromium.org/2645008/diff/1/5#newcode52
crypto.h:52: // Wraps (encrpyts) the vault keyset with the given wrapper
On 2010/06/09 19:08:12, Will Drewry wrote:
> typo

Done.

http://codereview.chromium.org/2645008/diff/1/5#newcode61
crypto.h:61: const SecureBlob& vault_wrapper,
On 2010/06/09 19:08:12, Will Drewry wrote:
> is there some weird tabs v spaces thing going on here or are these arguments
and
> hte arguments for the next prototype aligned wrong?

No, I renamed the method and neglected to fix the indentation.

http://codereview.chromium.org/2645008/diff/1/5#newcode138
crypto.h:138: }
On 2010/06/09 19:08:12, Will Drewry wrote:
> // namespace cryptohome

Done.

http://codereview.chromium.org/2645008/diff/1/6
File crypto_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/1/6#newcode46
crypto_unittest.cc:46: protected:
On 2010/06/09 19:08:12, Will Drewry wrote:
> style: you can probably just leave out the empty sections
> but under private you could still stop COPY_AND_ASSIGN.

Done.

http://codereview.chromium.org/2645008/diff/1/6#newcode127
crypto_unittest.cc:127: FilePath
salt_path(FilePath(kImageDir).Append("crypto_test_salt"));
On 2010/06/09 19:08:12, Will Drewry wrote:
> does FilePath or file_util support mocking out the filesystem?

I suppose it could be mocked for some more detailed testing.

http://codereview.chromium.org/2645008/diff/1/7
File cryptohome.cc (right):

http://codereview.chromium.org/2645008/diff/1/7#newcode86
cryptohome.cc:86: std::string* old_password_out = NULL) {
On 2010/06/09 19:08:12, Will Drewry wrote:
> style: this is still wrong :/  Default values aren't really allowed style-wise
> afaik.  (Maybe I'm just wrong :)  Callers should either explicitly pass in
NULL
> or there should be two functions:
> GetUsernamePassword(...) -> calls ->
> GetUsernamePasswords(..., old_password_out)
> 
> That would keep the callsites intact and let a migrate call path just call the
> underlying function.
> 
> A TODO would be nice for this cl - but since I don't see any callers using it,
> it might be an easy enough change to pull in.

Done.

http://codereview.chromium.org/2645008/diff/1/14
File make_tests.sh (right):

http://codereview.chromium.org/2645008/diff/1/14#newcode2
make_tests.sh:2: # Copyright (c) 2009 The Chromium OS Authors. All rights
reserved.
On 2010/06/09 19:08:12, Will Drewry wrote:
> nit: 2010

Done.

http://codereview.chromium.org/2645008/diff/1/15
File mount.cc (right):

http://codereview.chromium.org/2645008/diff/1/15#newcode61
mount.cc:61: if (crypto_)
On 2010/06/09 19:08:12, Will Drewry wrote:
> Do you want to delete crypto_ and platform_ even if they were passed in by an
> external caller?  It means it can't be used like:
> MockPlatform platform;
> MockCrypto crypto;
> Mount m(..., &crypto, &platform);
> 
> If you go with the default_impl_ scoped_ptr, then you can avoid having to take
> ownership of the pointer or track if you did.  
> 
> If you want it taking ownership, please explicitly state that it does in the
> comment on Mount() in mount.h

Done.

http://codereview.chromium.org/2645008/diff/1/16
File mount.h (right):

http://codereview.chromium.org/2645008/diff/1/16#newcode291
mount.h:291: Crypto* crypto_;
On 2010/06/09 19:08:12, Will Drewry wrote:
> scoped_ptr<Crypto> crypto_;
> 
> Or since you want to optionally pass in a pointer.  If you don't want to take
> ownership:
> scoped_ptr<Crypto> default_crypto_;
> Crypto *crypto_;
> 
> and have crypto_ = default_crypto_.get() unless it was passed in.

Done.

http://codereview.chromium.org/2645008/diff/1/16#newcode294
mount.h:294: Platform* platform_;
On 2010/06/09 19:08:12, Will Drewry wrote:
> ditto

Done.

http://codereview.chromium.org/2645008/diff/1/18
File platform.cc (right):

http://codereview.chromium.org/2645008/diff/1/18#newcode290
platform.cc:290: } // cryptohome
On 2010/06/09 19:08:12, Will Drewry wrote:
> // namespace cryptohome

Done.

http://codereview.chromium.org/2645008/diff/1/19
File platform.h (right):

http://codereview.chromium.org/2645008/diff/1/19#newcode5
platform.h:5: #ifndef PLATFORM_H_
On 2010/06/09 19:08:12, Will Drewry wrote:
> CRYPTOHOME_PLATFORM_H_?

Done.

http://codereview.chromium.org/2645008/diff/1/19#newcode32
platform.h:32: Platform(int mount_options, int umask, const std::string&
mtab_file,
On 2010/06/09 19:08:12, Will Drewry wrote:
> hrm so I was wrong about the cryptohome.cc thing:
>  
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...
> 
> But overloading here, isn't cool: 
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> To support injectable dependencies or non-default configs, I usually use
setters
> which should be called before any other function is used and note that in the
> comment.  If there is an Init() function, then I'll make that set a bool and
> have those setters be required prior to init (if appropriate).
> 
> Another options is to have the constructor call some default_setter if
> Plaform(bool use_defaults).  then if not, they need to be set.  I'm sure there
> are better ways too.

Done.

http://codereview.chromium.org/2645008/diff/1/19#newcode104
platform.h:104: }
On 2010/06/09 19:08:12, Will Drewry wrote:
> // namespace ...
> 

Done.

http://codereview.chromium.org/2645008/diff/1/22
File secure_blob_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/1/22#newcode14
secure_blob_unittest.cc:14: using namespace chromeos;
On 2010/06/09 19:08:12, Will Drewry wrote:
> style: pulling in a whole namespace is generally not ok.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Done.

http://codereview.chromium.org/2645008/diff/1/22#newcode18
secure_blob_unittest.cc:18: void SetUp() {
On 2010/06/09 19:08:12, Will Drewry wrote:
> Should this be before public:?

Done.

http://codereview.chromium.org/2645008/diff/1/22#newcode38
secure_blob_unittest.cc:38: 
On 2010/06/09 19:08:12, Will Drewry wrote:
> nit: empty sections not really needed.

Done.

http://codereview.chromium.org/2645008/diff/1/29
File username_passkey.h (right):

http://codereview.chromium.org/2645008/diff/1/29#newcode24
username_passkey.h:24: const chromeos::Blob& salt);
On 2010/06/09 19:08:12, Will Drewry wrote:
> style: function overloading should be avoided :/

Done.

http://codereview.chromium.org/2645008/diff/1/31
File vault_keyset.cc (right):

http://codereview.chromium.org/2645008/diff/1/31#newcode73
vault_keyset.cc:73: Min(fek_sig_.size(), sizeof(keys.fek_sig)));
On 2010/06/09 19:08:12, Will Drewry wrote:
> size() and sizeof() aren't signed 32-bit ints but vector::size_type and size_t
> and memcpy expects a size_t.  If your Min function is explicitly only for
these
> cases, then it would make more sense to have it compare size_t which would
avoid
> potential truncation if we go 64-bit and any other casting weirdness.
> 
> Noot that those are likely in any of these scenarios :), but it'd just be
> cleaner to not force a type conversion when you know in advance you don't need
> one.
> 
> std::min might also work for you -- it is just a templated min test.  It is
> subject to some weirdness but should work fine when comparing to size_ts.

Done.

http://codereview.chromium.org/2645008/diff/1/33
File vault_keyset_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/1/33#newcode16
vault_keyset_unittest.cc:16: using namespace chromeos;
On 2010/06/09 19:08:12, Will Drewry wrote:
> style: dont pull in a full namespace; alias any needed names

Done.

http://codereview.chromium.org/2645008/diff/1/33#newcode20
vault_keyset_unittest.cc:20: void SetUp() {
On 2010/06/09 19:08:12, Will Drewry wrote:
> nit: outside of declared section 

Done.

http://codereview.chromium.org/2645008/diff/1/33#newcode39
vault_keyset_unittest.cc:39: protected:
On 2010/06/09 19:08:12, Will Drewry wrote:
> nit: protected and private not really needed.

Done.

[Will Drewry, 2010-06-11 06:40:04]

LGTM with a few style and typo nits

http://codereview.chromium.org/2645008/diff/19001/20003
File crypto.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20003#newcode16
crypto.cc:16: #include "chromeos/utility.h"
fwiw, ditto with <chromeos/utility.h> when we get around to it.

http://codereview.chromium.org/2645008/diff/19001/20004
File crypto.h (right):

http://codereview.chromium.org/2645008/diff/19001/20004#newcode13
crypto.h:13: #include "base/basictypes.h"
Nothing to change now unless you want to, but msb@ raise the point that as we
properly break up our tree, we should be able to start referencing external
includes with <>.  E.g.,
#include <base/basictypes.h> 

(Of course, we should also move that to chrome-base ;)

http://codereview.chromium.org/2645008/diff/19001/20004#newcode30
crypto.h:30: Crypto(const std::string& entropy_source);
Any chance of using a setter and/or an Init() function?
E.g.,
  set_entropy_source(...)

http://codereview.chromium.org/2645008/diff/19001/20005
File crypto_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20005#newcode20
crypto_unittest.cc:20: using namespace chromeos;
full namespace inclusion here and below. but at least it's a unittest :)

http://codereview.chromium.org/2645008/diff/19001/20006
File cryptohome.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20006#newcode19
cryptohome.cc:19: #include <chromeos/dbus/dbus.h>
here <> is used a little more properly and how we want to use it long term.

http://codereview.chromium.org/2645008/diff/19001/20014
File mount.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20014#newcode30
mount.cc:30: const std::string kIncognitoUser = "incognito";
is this approach still in use in any other code?  Maybe a todo to kill the
incognito-user path would be good.

http://codereview.chromium.org/2645008/diff/19001/20033
File vault_keyset_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20033#newcode78
vault_keyset_unittest.cc:78: TEST_F(VaultKeysetTest, DeerializeTest) {
Deer!

[fes, 2010-06-11 16:31:59]

Updated, with modifications to other files to reflect the new proposed include
conventions.  I also updated the include ordering to reflect the C first, C++
second convention in the style guide.

http://codereview.chromium.org/2645008/diff/19001/20003
File crypto.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20003#newcode16
crypto.cc:16: #include "chromeos/utility.h"
On 2010/06/11 06:40:04, Will Drewry wrote:
> fwiw, ditto with <chromeos/utility.h> when we get around to it.

Done.

http://codereview.chromium.org/2645008/diff/19001/20004
File crypto.h (right):

http://codereview.chromium.org/2645008/diff/19001/20004#newcode13
crypto.h:13: #include "base/basictypes.h"
On 2010/06/11 06:40:04, Will Drewry wrote:
> Nothing to change now unless you want to, but msb@ raise the point that as we
> properly break up our tree, we should be able to start referencing external
> includes with <>.  E.g.,
> #include <base/basictypes.h> 
> 
> (Of course, we should also move that to chrome-base ;)

Done.

http://codereview.chromium.org/2645008/diff/19001/20004#newcode30
crypto.h:30: Crypto(const std::string& entropy_source);
On 2010/06/11 06:40:04, Will Drewry wrote:
> Any chance of using a setter and/or an Init() function?
> E.g.,
>   set_entropy_source(...)

Done.

http://codereview.chromium.org/2645008/diff/19001/20005
File crypto_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20005#newcode20
crypto_unittest.cc:20: using namespace chromeos;
On 2010/06/11 06:40:04, Will Drewry wrote:
> full namespace inclusion here and below. but at least it's a unittest :)

Done.

http://codereview.chromium.org/2645008/diff/19001/20006
File cryptohome.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20006#newcode19
cryptohome.cc:19: #include <chromeos/dbus/dbus.h>
On 2010/06/11 06:40:04, Will Drewry wrote:
> here <> is used a little more properly and how we want to use it long term.

Done.

http://codereview.chromium.org/2645008/diff/19001/20014
File mount.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20014#newcode30
mount.cc:30: const std::string kIncognitoUser = "incognito";
On 2010/06/11 06:40:04, Will Drewry wrote:
> is this approach still in use in any other code?  Maybe a todo to kill the
> incognito-user path would be good.

Done.

http://codereview.chromium.org/2645008/diff/19001/20033
File vault_keyset_unittest.cc (right):

http://codereview.chromium.org/2645008/diff/19001/20033#newcode78
vault_keyset_unittest.cc:78: TEST_F(VaultKeysetTest, DeerializeTest) {
On 2010/06/11 06:40:04, Will Drewry wrote:
> Deer!

Done.