AU: Bzip2 utility functions that mirror Gzip utility functions.

AU: Bzip2 utility functions that mirror Gzip utility functions.

Also, Convert the gzip_unittest into a typed test and use the same
test cases for bzip2.

[Daniel Erat, 2010-03-11 16:42:03]

http://codereview.chromium.org/855002/diff/1/3
File src/platform/update_engine/bzip.cc (right):

http://codereview.chromium.org/855002/diff/1/3#newcode1
src/platform/update_engine/bzip.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
nit: update copyright year

http://codereview.chromium.org/855002/diff/1/3#newcode9
src/platform/update_engine/bzip.cc:9: #include "chromeos/obsolete_logging.h"
i don't think you need this here either

http://codereview.chromium.org/855002/diff/1/3#newcode23
src/platform/update_engine/bzip.cc:23: *out_size = 0;
this seems pretty tricky; i can imagine a caller seeing that the data is 0 bytes
and not bothering to check the pointer and free it later.  is 0-byte input data
something that you actually expect to handle?  my copy of bunzip2 barfs when i
give it an empty file, although maybe that's different.  if you need to handle
these, how would you feel about setting *out to NULL and documenting that in the
header?

(this and most of the other comments also apply to the compression code.)

http://codereview.chromium.org/855002/diff/1/3#newcode27
src/platform/update_engine/bzip.cc:27: size_t buf_size = in_size * 2;  // Decent
starting point(?)
let the caller pass in a hint about the size of the original data, maybe.  i
forget, is this something that's available in the update proto?

http://codereview.chromium.org/855002/diff/1/3#newcode39
src/platform/update_engine/bzip.cc:39: TEST_AND_RETURN_FALSE(rc ==
BZ_OUTBUFF_FULL || rc == BZ_OK);
the names of these macros still confuse me (sorry, i think i complained about
CHECK_AND vs. TEST_AND before) -- it sounds like it'll return false if the
condition is true, or maybe unconditionally, when it actually does it if the
condition is false.  i'm not sure what'd be better; maybe
IF_TRUE_RETURN_FALSE()?

http://codereview.chromium.org/855002/diff/1/3#newcode43
src/platform/update_engine/bzip.cc:43: *out_size = data_size;
how much should you be concerned about potentially returning a buffer that's
almost twice as large as what's actually needed?  is it worth it to use "*out =
realloc(buf, data_size)"?  it sounds at least possible that it might avoid doing
a copy in this case, and just shrink the previous allocation...

(i see that this isn't a concern when using the helper functions since they do
another copy, but this function is outside-facing right now.)

http://codereview.chromium.org/855002/diff/1/3#newcode99
src/platform/update_engine/bzip.cc:99: out->insert(out->end(), out_buf, out_buf
+ out_size);
does the appending behavior used here and in the other helpers ever make sense? 
if not, it's safer to clear 'out' first

http://codereview.chromium.org/855002/diff/1/3#newcode105
src/platform/update_engine/bzip.cc:105: TEST_AND_RETURN_FALSE(out);
there's a lot of duplicated code between these methods.  do you actually need
Bzip*Data() functions in their present forms, or could you make them take
vector<char> for their out-params?

http://codereview.chromium.org/855002/diff/1/4
File src/platform/update_engine/bzip.h (right):

http://codereview.chromium.org/855002/diff/1/4#newcode13
src/platform/update_engine/bzip.h:13: bool BzipCompressData(const char* const
in, const size_t in_size,
the extra consts on the in-params seem superfluous (or at least, it's very
uncommon to do that).  your call, though

http://codereview.chromium.org/855002/diff/1/5
File src/platform/update_engine/gzip_unittest.cc (right):

http://codereview.chromium.org/855002/diff/1/5#newcode21
src/platform/update_engine/gzip_unittest.cc:21: class ZipTest : public
::testing::Test {
rename the file?

[adlr, 2010-03-11 22:35:44]

ready for another look. thanks!

http://codereview.chromium.org/855002/diff/1/3
File src/platform/update_engine/bzip.cc (right):

http://codereview.chromium.org/855002/diff/1/3#newcode1
src/platform/update_engine/bzip.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2010/03/11 16:42:03, Daniel Erat wrote:
> nit: update copyright year

Done.

http://codereview.chromium.org/855002/diff/1/3#newcode9
src/platform/update_engine/bzip.cc:9: #include "chromeos/obsolete_logging.h"
On 2010/03/11 16:42:03, Daniel Erat wrote:
> i don't think you need this here either

Done.

http://codereview.chromium.org/855002/diff/1/3#newcode23
src/platform/update_engine/bzip.cc:23: *out_size = 0;
On 2010/03/11 16:42:03, Daniel Erat wrote:
> this seems pretty tricky; i can imagine a caller seeing that the data is 0
bytes
> and not bothering to check the pointer and free it later.  is 0-byte input
data
> something that you actually expect to handle?  my copy of bunzip2 barfs when i
> give it an empty file, although maybe that's different.  if you need to handle
> these, how would you feel about setting *out to NULL and documenting that in
the
> header?
> 
> (this and most of the other comments also apply to the compression code.)

n/a

http://codereview.chromium.org/855002/diff/1/3#newcode27
src/platform/update_engine/bzip.cc:27: size_t buf_size = in_size * 2;  // Decent
starting point(?)
On 2010/03/11 16:42:03, Daniel Erat wrote:
> let the caller pass in a hint about the size of the original data, maybe.  i
> forget, is this something that's available in the update proto?

This code isn't called on the client. Rather, the client will use a
bzip-extent-writer to do the decompression and that won't cause the entire file
to be decompressed in one fell swoop, but rather decompressed in pieces, which
uses less memory.

I actually don't call the bzip decompression code anywhere. I wrote it b/c I was
writing the compression code and figured I should just write a matching
decompression algo while the bzip library code was fresh in my mind.

In places where the compression function is called, I don't think I have any
hints available.

http://codereview.chromium.org/855002/diff/1/3#newcode39
src/platform/update_engine/bzip.cc:39: TEST_AND_RETURN_FALSE(rc ==
BZ_OUTBUFF_FULL || rc == BZ_OK);
On 2010/03/11 16:42:03, Daniel Erat wrote:
> the names of these macros still confuse me (sorry, i think i complained about
> CHECK_AND vs. TEST_AND before) -- it sounds like it'll return false if the
> condition is true, or maybe unconditionally, when it actually does it if the
> condition is false.  i'm not sure what'd be better; maybe
> IF_TRUE_RETURN_FALSE()?

Yeah, you did complain once already. I don't mind what it's called. How about I
put this on the list of future CLs (it's an actual list, not a figure of
speech), since it will affect a lot of files.

http://codereview.chromium.org/855002/diff/1/3#newcode43
src/platform/update_engine/bzip.cc:43: *out_size = data_size;
On 2010/03/11 16:42:03, Daniel Erat wrote:
> how much should you be concerned about potentially returning a buffer that's
> almost twice as large as what's actually needed?  is it worth it to use "*out
=
> realloc(buf, data_size)"?  it sounds at least possible that it might avoid
doing
> a copy in this case, and just shrink the previous allocation...
> 
> (i see that this isn't a concern when using the helper functions since they do
> another copy, but this function is outside-facing right now.)

Done.

http://codereview.chromium.org/855002/diff/1/3#newcode99
src/platform/update_engine/bzip.cc:99: out->insert(out->end(), out_buf, out_buf
+ out_size);
On 2010/03/11 16:42:03, Daniel Erat wrote:
> does the appending behavior used here and in the other helpers ever make
sense? 
> if not, it's safer to clear 'out' first

Done.

http://codereview.chromium.org/855002/diff/1/3#newcode105
src/platform/update_engine/bzip.cc:105: TEST_AND_RETURN_FALSE(out);
On 2010/03/11 16:42:03, Daniel Erat wrote:
> there's a lot of duplicated code between these methods.  do you actually need
> Bzip*Data() functions in their present forms, or could you make them take
> vector<char> for their out-params?

i think i got rid of all the code duplication

http://codereview.chromium.org/855002/diff/1/4
File src/platform/update_engine/bzip.h (right):

http://codereview.chromium.org/855002/diff/1/4#newcode13
src/platform/update_engine/bzip.h:13: bool BzipCompressData(const char* const
in, const size_t in_size,
On 2010/03/11 16:42:03, Daniel Erat wrote:
> the extra consts on the in-params seem superfluous (or at least, it's very
> uncommon to do that).  your call, though

I guess I'm in the camp that says that more correctly labeling what is const is
a good thing, even if it's more verbose.

http://codereview.chromium.org/855002/diff/1/5
File src/platform/update_engine/gzip_unittest.cc (right):

http://codereview.chromium.org/855002/diff/1/5#newcode21
src/platform/update_engine/gzip_unittest.cc:21: class ZipTest : public
::testing::Test {
On 2010/03/11 16:42:03, Daniel Erat wrote:
> rename the file?

Done.

[Daniel Erat, 2010-03-11 23:25:04]

LGTM

http://codereview.chromium.org/855002/diff/9004/10003
File src/platform/update_engine/bzip.h (right):

http://codereview.chromium.org/855002/diff/9004/10003#newcode13
src/platform/update_engine/bzip.h:13: int BzipBuffToBuffDecompress(char* out,
Are these supposed to be outside-facing?  If not, move them to the .cc file and
put them into an anon namespace (maybe the template needs to stay in the header,
though).