Refactor CT client tool

[Ben Laurie (Google), 2012-10-04 11:21:45]

LGTM

https://codereview.appspot.com/6591070/diff/6001/src/client/client.h
File src/client/client.h (right):

https://codereview.appspot.com/6591070/diff/6001/src/client/client.h#newcode7
src/client/client.h:7: // A generic client.
Surely a truly generic client would not have server/port - e.g. it might work by
email or jabber?

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc
File src/client/ct.cc (right):

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc#newcode258
src/client/ct.cc:258: std::ios::in|std::ios::binary);
Space around |.

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc#newcode262
src/client/ct.cc:262: std::ios::out|std::ios::binary);
ditto

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc#newcode294
src/client/ct.cc:294: //  2: connection error
Use an enum?

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc#newcode336
src/client/ct.cc:336: // 2: unable to connect to server
You mean exit values? Also, what about abnormal exit?

https://codereview.appspot.com/6591070/diff/6001/src/client/log_client.cc
File src/client/log_client.cc (right):

https://codereview.appspot.com/6591070/diff/6001/src/client/log_client.cc#new...
src/client/log_client.cc:116: int version = 256;
Comment that this should be a number that is not a valid version. -1 might be
better?

In any case, would we not expect an error return if this is not set?

https://codereview.appspot.com/6591070/diff/6001/src/client/log_client.cc#new...
src/client/log_client.cc:135: int format = 256;
See above.

https://codereview.appspot.com/6591070/diff/6001/src/client/log_client.cc#new...
src/client/log_client.cc:152: size_t packet_length = 0;
See above.

https://codereview.appspot.com/6591070/diff/6001/src/client/ssl_client.h
File src/client/ssl_client.h (right):

https://codereview.appspot.com/6591070/diff/6001/src/client/ssl_client.h#newc...
src/client/ssl_client.h:52: // -1 - no valid cert
These return values are defined by OpenSSL?

https://codereview.appspot.com/6591070/diff/6001/src/client/ssl_client.h#newc...
src/client/ssl_client.h:58: // 0 - no valid token
See above

https://codereview.appspot.com/6591070/diff/6001/src/server/ct-server.cc
File src/server/ct-server.cc (right):

https://codereview.appspot.com/6591070/diff/6001/src/server/ct-server.cc#newc...
src/server/ct-server.cc:462: int format = 256;
Use -1?

https://codereview.appspot.com/6591070/diff/6001/src/test/sslconnect_test.sh
File src/test/sslconnect_test.sh (right):

https://codereview.appspot.com/6591070/diff/6001/src/test/sslconnect_test.sh#...
src/test/sslconnect_test.sh:65: test_connect $cert_dir $hash_dir $log_server $ca
$port $retcode $strict;
What's the ; for?

https://codereview.appspot.com/6591070/diff/6001/src/util/util.cc
File src/util/util.cc (right):

https://codereview.appspot.com/6591070/diff/6001/src/util/util.cc#newcode52
src/util/util.cc:52: ret.push_back(byte_delimiter);
I prefer to start the loop at 0 and put

if (i != 0) ret.push_back(byte_delimiter);

[ekasper, 2012-10-04 13:14:54]

PTAL

https://codereview.appspot.com/6591070/diff/6001/src/client/client.h
File src/client/client.h (right):

https://codereview.appspot.com/6591070/diff/6001/src/client/client.h#newcode7
src/client/client.h:7: // A generic client.
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> Surely a truly generic client would not have server/port - e.g. it might work
by
> email or jabber?

I didn't mean a generic CT client; clarified.

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc
File src/client/ct.cc (right):

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc#newcode258
src/client/ct.cc:258: std::ios::in|std::ios::binary);
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> Space around |.

Done.

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc#newcode262
src/client/ct.cc:262: std::ios::out|std::ios::binary);
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> ditto

Done.

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc#newcode294
src/client/ct.cc:294: //  2: connection error
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> Use an enum?

Yep, done.

https://codereview.appspot.com/6591070/diff/6001/src/client/ct.cc#newcode336
src/client/ct.cc:336: // 2: unable to connect to server
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> You mean exit values? Also, what about abnormal exit?

Sigh, yeah, abnormal exit depends on what CHECK decides to exit with... 

But I just thought of something better (I think). PTAL.

https://codereview.appspot.com/6591070/diff/6001/src/client/log_client.cc
File src/client/log_client.cc (right):

https://codereview.appspot.com/6591070/diff/6001/src/client/log_client.cc#new...
src/client/log_client.cc:116: int version = 256;
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> Comment that this should be a number that is not a valid version. -1 might be
> better?

256 is illegal too since we have to fit in one byte, but done.
> 
> In any case, would we not expect an error return if this is not set?

It's just to make the compiler happy...

https://codereview.appspot.com/6591070/diff/6001/src/client/log_client.cc#new...
src/client/log_client.cc:135: int format = 256;
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> See above.

Done.

https://codereview.appspot.com/6591070/diff/6001/src/client/log_client.cc#new...
src/client/log_client.cc:152: size_t packet_length = 0;
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> See above.

Done.

https://codereview.appspot.com/6591070/diff/6001/src/client/ssl_client.h
File src/client/ssl_client.h (right):

https://codereview.appspot.com/6591070/diff/6001/src/client/ssl_client.h#newc...
src/client/ssl_client.h:52: // -1 - no valid cert
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> These return values are defined by OpenSSL?

Oops, these were out of date; updated.

https://codereview.appspot.com/6591070/diff/6001/src/client/ssl_client.h#newc...
src/client/ssl_client.h:58: // 0 - no valid token
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> See above

Updated.

https://codereview.appspot.com/6591070/diff/6001/src/test/sslconnect_test.sh
File src/test/sslconnect_test.sh (right):

https://codereview.appspot.com/6591070/diff/6001/src/test/sslconnect_test.sh#...
src/test/sslconnect_test.sh:65: test_connect $cert_dir $hash_dir $log_server $ca
$port $retcode $strict;
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> What's the ; for?

typo

https://codereview.appspot.com/6591070/diff/6001/src/util/util.cc
File src/util/util.cc (right):

https://codereview.appspot.com/6591070/diff/6001/src/util/util.cc#newcode52
src/util/util.cc:52: ret.push_back(byte_delimiter);
On 2012/10/04 11:21:46, Ben Laurie (Google) wrote:
> I prefer to start the loop at 0 and put
> 
> if (i != 0) ret.push_back(byte_delimiter);

Done.

[Ben Laurie (Google), 2012-10-04 13:36:32]

LGTM

[ekasper, 2012-10-04 14:40:21]

[+alcutter FYI]

Thanks, pushed.