Issue 5531049: First part of client
|
Created:
14 years, 5 months ago by Ben Laurie (Google) Modified:
14 years, 5 months ago Reviewers:
ekasper Visibility:
Public. |
Patch Set 1 #
Total comments: 17
MessagesTotal messages: 7
Sign in to reply to this message.
http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc File src/client/ct.cc (right): http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode52 src/client/ct.cc:52: } Blank line. Also, document the wire format briefly, please. http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode53 src/client/ct.cc:53: void uploadBundle(const std::string &bundle) { Verify that the bundle size is within limits before attempting to write anything. (Print an appropriate error if it isn't?) http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode55 src/client/ct.cc:55: writeLength(bundle.length(), 3); Make the magic 3 a symbolic constant? http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode66 src/client/ct.cc:66: } blank line http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode71 src/client/ct.cc:71: } blank line http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode85 src/client/ct.cc:85: }; blank line http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode98 src/client/ct.cc:98: unsigned port = atoi(argv[3]); include stdlib.h for atoi http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode103 src/client/ct.cc:103: std::ifstream in(file); Spit out a file-not-found error if !in.is_open()?
Sign in to reply to this message.
Also, while I'm nitpicking (sorry)... http://codereview.appspot.com/5531049/diff/1/src/client/Makefile File src/client/Makefile (right): http://codereview.appspot.com/5531049/diff/1/src/client/Makefile#newcode2 src/client/Makefile:2: LIBS= -lcrypto unused
Sign in to reply to this message.
But for some reason it created a new issue: http://codereview.appspot.com/5528052 http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc File src/client/ct.cc (right): http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode52 src/client/ct.cc:52: } On 2012/01/09 19:12:49, ekasper wrote: > Blank line. Also, document the wire format briefly, please. Done. http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode53 src/client/ct.cc:53: void uploadBundle(const std::string &bundle) { On 2012/01/09 19:12:49, ekasper wrote: > Verify that the bundle size is within limits before attempting to write > anything. (Print an appropriate error if it isn't?) Done. http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode66 src/client/ct.cc:66: } On 2012/01/09 19:12:49, ekasper wrote: > blank line Done. http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode71 src/client/ct.cc:71: } On 2012/01/09 19:12:49, ekasper wrote: > blank line Done. http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode85 src/client/ct.cc:85: }; On 2012/01/09 19:12:49, ekasper wrote: > blank line Done. http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode98 src/client/ct.cc:98: unsigned port = atoi(argv[3]); On 2012/01/09 19:12:49, ekasper wrote: > include stdlib.h for atoi Done. http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode103 src/client/ct.cc:103: std::ifstream in(file); On 2012/01/09 19:12:49, ekasper wrote: > Spit out a file-not-found error if !in.is_open()? Done.
Sign in to reply to this message.
LGTM Could you also update the maximum bundle length in the spec to match the code? (Btw you have to give upload.py -i 12345678 to update an existing issue no 12345678.) On 2012/01/09 21:38:45, Ben Laurie (Google) wrote: > But for some reason it created a new issue: > > http://codereview.appspot.com/5528052 > > http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc > File src/client/ct.cc (right): > > http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode52 > src/client/ct.cc:52: } > On 2012/01/09 19:12:49, ekasper wrote: > > Blank line. Also, document the wire format briefly, please. > > Done. > > http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode53 > src/client/ct.cc:53: void uploadBundle(const std::string &bundle) { > On 2012/01/09 19:12:49, ekasper wrote: > > Verify that the bundle size is within limits before attempting to write > > anything. (Print an appropriate error if it isn't?) > > Done. > > http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode66 > src/client/ct.cc:66: } > On 2012/01/09 19:12:49, ekasper wrote: > > blank line > > Done. > > http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode71 > src/client/ct.cc:71: } > On 2012/01/09 19:12:49, ekasper wrote: > > blank line > > Done. > > http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode85 > src/client/ct.cc:85: }; > On 2012/01/09 19:12:49, ekasper wrote: > > blank line > > Done. > > http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode98 > src/client/ct.cc:98: unsigned port = atoi(argv[3]); > On 2012/01/09 19:12:49, ekasper wrote: > > include stdlib.h for atoi > > Done. > > http://codereview.appspot.com/5531049/diff/1/src/client/ct.cc#newcode103 > src/client/ct.cc:103: std::ifstream in(file); > On 2012/01/09 19:12:49, ekasper wrote: > > Spit out a file-not-found error if !in.is_open()? > > Done.
Sign in to reply to this message.
http://codereview.appspot.com/5531049/diff/1/src/client/Makefile File src/client/Makefile (right): http://codereview.appspot.com/5531049/diff/1/src/client/Makefile#newcode2 src/client/Makefile:2: LIBS= -lcrypto On 2012/01/09 19:31:59, ekasper wrote: > unused It will be, though :-)
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
