AU: SplitWriter class for parsing our full update files.

AU: SplitWriter class for parsing our full update files.

Full updates now include data for two partitions (kernel + rootfs).
This CL adds a new SplitFileWriter class which takes the stream of
data, which is expected to be in our full update format (8 bytes big
endian uint64_t of the first partition size, followed by first
partition data, followed by second partition size). It parses the
size, then writes the data to each of two FileWriter classes.

BUG=None
TEST=Attached unittest

[Daniel Erat, 2010-04-29 06:37:15]

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

http://codereview.chromium.org/1800009/diff/1/3#newcode15
src/platform/update_engine/split_file_writer.cc:15: // Error
these comments don't seem to add much; the next line is a LOG(ERROR)

http://codereview.chromium.org/1800009/diff/1/3#newcode31
src/platform/update_engine/split_file_writer.cc:31:
CHECK_EQ(static_cast<size_t>(static_cast<int>(count)), count);
what's the reason for this weird check?  you're trying to make sure that the
current value of count fits inside of an int?  why not just make the argument an
int instead, then?

http://codereview.chromium.org/1800009/diff/1/3#newcode47
src/platform/update_engine/split_file_writer.cc:47: if (bytes_written_ <
static_cast<off_t>(sizeof(uint64_t))) {
this block would benefit from a comment stating that it's trying to read the
initial number containing the size of the first partition

http://codereview.chromium.org/1800009/diff/1/3#newcode54
src/platform/update_engine/split_file_writer.cc:54: bytes = static_cast<const
void*>(
you shouldn't need to cast to void* (ditto later too)

as a larger question, is there a reason why Write() takes void* instead of
char*?  void* means "i don't care about the type of this pointer", so if you're
needing to cast it to char*, it probably means that that statement isn't true.

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

http://codereview.chromium.org/1800009/diff/1/4#newcode20
src/platform/update_engine/split_file_writer.h:20: SplitFileWriter(FileWriter*
first, FileWriter* second) : first_(first),
indenting for c'tors should be:

SplitFileWriter(args)
    : first_(first),
      first_length_(0),
      etc.

http://codereview.chromium.org/1800009/diff/1/4#newcode44
src/platform/update_engine/split_file_writer.h:44: FileWriter* const first_;
call this something like first_writer_ instead

http://codereview.chromium.org/1800009/diff/1/4#newcode54
src/platform/update_engine/split_file_writer.h:54: off_t bytes_written_;
i find bytes_written_ to be a somewhat misleading name here.  it's not the
number of bytes that this class has written, it's the number of bytes that have
been passed to the Write() method, and some of those bytes don't get written at
all.  this at least deserves a comment, but naming it something like
'bytes_handled_' would be even better.

[adlr, 2010-04-29 20:22:33]

please have another look. thanks!

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

http://codereview.chromium.org/1800009/diff/1/3#newcode15
src/platform/update_engine/split_file_writer.cc:15: // Error
On 2010/04/29 06:37:15, Daniel Erat wrote:
> these comments don't seem to add much; the next line is a LOG(ERROR)

Done.

http://codereview.chromium.org/1800009/diff/1/3#newcode31
src/platform/update_engine/split_file_writer.cc:31:
CHECK_EQ(static_cast<size_t>(static_cast<int>(count)), count);
On 2010/04/29 06:37:15, Daniel Erat wrote:
> what's the reason for this weird check?  you're trying to make sure that the
> current value of count fits inside of an int?  why not just make the argument
an
> int instead, then?

Yeah, it was to make sure it didn't overflow. I realized I was trying to have
FileWriter mimic the open(), write(), close() sys calls, so the right answer is
to make Write() return ssize_t.

http://codereview.chromium.org/1800009/diff/1/3#newcode47
src/platform/update_engine/split_file_writer.cc:47: if (bytes_written_ <
static_cast<off_t>(sizeof(uint64_t))) {
On 2010/04/29 06:37:15, Daniel Erat wrote:
> this block would benefit from a comment stating that it's trying to read the
> initial number containing the size of the first partition

Done.

http://codereview.chromium.org/1800009/diff/1/3#newcode54
src/platform/update_engine/split_file_writer.cc:54: bytes = static_cast<const
void*>(
On 2010/04/29 06:37:15, Daniel Erat wrote:
> you shouldn't need to cast to void* (ditto later too)
> 
> as a larger question, is there a reason why Write() takes void* instead of
> char*?  void* means "i don't care about the type of this pointer", so if
you're
> needing to cast it to char*, it probably means that that statement isn't true.

Talking to some people, I'm inclined to keep void*. Here are the pros/cons of
switching to char*:

+ Less casting inside Write() functions b/c you can do arithmetic on char*
– Some more casting outside of Write() functions, as non-char*s need to be
casted to char*, whereas they can be implicitly cast to void*.
– Breaks convention set by write(), pwrite(), memcmp(), etc. of using void* to
point to an array of bytes.

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

http://codereview.chromium.org/1800009/diff/1/4#newcode20
src/platform/update_engine/split_file_writer.h:20: SplitFileWriter(FileWriter*
first, FileWriter* second) : first_(first),
On 2010/04/29 06:37:15, Daniel Erat wrote:
> indenting for c'tors should be:
> 
> SplitFileWriter(args)
>     : first_(first),
>       first_length_(0),
>       etc.

Done.

http://codereview.chromium.org/1800009/diff/1/4#newcode44
src/platform/update_engine/split_file_writer.h:44: FileWriter* const first_;
On 2010/04/29 06:37:15, Daniel Erat wrote:
> call this something like first_writer_ instead

Done.

http://codereview.chromium.org/1800009/diff/1/4#newcode54
src/platform/update_engine/split_file_writer.h:54: off_t bytes_written_;
On 2010/04/29 06:37:15, Daniel Erat wrote:
> i find bytes_written_ to be a somewhat misleading name here.  it's not the
> number of bytes that this class has written, it's the number of bytes that
have
> been passed to the Write() method, and some of those bytes don't get written
at
> all.  this at least deserves a comment, but naming it something like
> 'bytes_handled_' would be even better.

changed to bytes_received_

[Daniel Erat, 2010-04-29 20:44:54]

LGTM

On Thu, Apr 29, 2010 at 1:22 PM,  <adlr@chromium.org> wrote:
> please have another look. thanks!
>
>
> http://codereview.chromium.org/1800009/diff/1/3
> File src/platform/update_engine/split_file_writer.cc (right):
>
> http://codereview.chromium.org/1800009/diff/1/3#newcode15
> src/platform/update_engine/split_file_writer.cc:15: // Error
> On 2010/04/29 06:37:15, Daniel Erat wrote:
>>
>> these comments don't seem to add much; the next line is a LOG(ERROR)
>
> Done.
>
> http://codereview.chromium.org/1800009/diff/1/3#newcode31
> src/platform/update_engine/split_file_writer.cc:31:
> CHECK_EQ(static_cast<size_t>(static_cast<int>(count)), count);
> On 2010/04/29 06:37:15, Daniel Erat wrote:
>>
>> what's the reason for this weird check?  you're trying to make sure
>
> that the
>>
>> current value of count fits inside of an int?  why not just make the
>
> argument an
>>
>> int instead, then?
>
> Yeah, it was to make sure it didn't overflow. I realized I was trying to
> have FileWriter mimic the open(), write(), close() sys calls, so the
> right answer is to make Write() return ssize_t.
>
> http://codereview.chromium.org/1800009/diff/1/3#newcode47
> src/platform/update_engine/split_file_writer.cc:47: if (bytes_written_ <
> static_cast<off_t>(sizeof(uint64_t))) {
> On 2010/04/29 06:37:15, Daniel Erat wrote:
>>
>> this block would benefit from a comment stating that it's trying to
>
> read the
>>
>> initial number containing the size of the first partition
>
> Done.
>
> http://codereview.chromium.org/1800009/diff/1/3#newcode54
> src/platform/update_engine/split_file_writer.cc:54: bytes =
> static_cast<const void*>(
> On 2010/04/29 06:37:15, Daniel Erat wrote:
>>
>> you shouldn't need to cast to void* (ditto later too)
>
>> as a larger question, is there a reason why Write() takes void*
>
> instead of
>>
>> char*?  void* means "i don't care about the type of this pointer", so
>
> if you're
>>
>> needing to cast it to char*, it probably means that that statement
>
> isn't true.
>
> Talking to some people, I'm inclined to keep void*. Here are the
> pros/cons of switching to char*:
>
> + Less casting inside Write() functions b/c you can do arithmetic on
> char*
> – Some more casting outside of Write() functions, as non-char*s need to
> be casted to char*, whereas they can be implicitly cast to void*.
> – Breaks convention set by write(), pwrite(), memcmp(), etc. of using
> void* to point to an array of bytes.
>
> http://codereview.chromium.org/1800009/diff/1/4
> File src/platform/update_engine/split_file_writer.h (right):
>
> http://codereview.chromium.org/1800009/diff/1/4#newcode20
> src/platform/update_engine/split_file_writer.h:20:
> SplitFileWriter(FileWriter* first, FileWriter* second) : first_(first),
> On 2010/04/29 06:37:15, Daniel Erat wrote:
>>
>> indenting for c'tors should be:
>
>> SplitFileWriter(args)
>>     : first_(first),
>>       first_length_(0),
>>       etc.
>
> Done.
>
> http://codereview.chromium.org/1800009/diff/1/4#newcode44
> src/platform/update_engine/split_file_writer.h:44: FileWriter* const
> first_;
> On 2010/04/29 06:37:15, Daniel Erat wrote:
>>
>> call this something like first_writer_ instead
>
> Done.
>
> http://codereview.chromium.org/1800009/diff/1/4#newcode54
> src/platform/update_engine/split_file_writer.h:54: off_t bytes_written_;
> On 2010/04/29 06:37:15, Daniel Erat wrote:
>>
>> i find bytes_written_ to be a somewhat misleading name here.  it's not
>
> the
>>
>> number of bytes that this class has written, it's the number of bytes
>
> that have
>>
>> been passed to the Write() method, and some of those bytes don't get
>
> written at
>>
>> all.  this at least deserves a comment, but naming it something like
>> 'bytes_handled_' would be even better.
>
> changed to bytes_received_
>
> http://codereview.chromium.org/1800009/show
>