AU: Extent writer utility classes

AU: Extent writer utility classes

These are similar to the FileWriter classes, but focus on writing to
extents within a file (or device) rather than writing a file in order.

Again there is a ExtentWriter interface from which all types of extent
writers interit.

There are also two basic extent writers: a direct extent writer that
writes data directly to the file descriptor, and a zero padding extent
writer that piggy-backs on another extent writer. When the
zero-padding extent writer is End()ed, it fills out the rest of the
extent with zeros.

Also, BzipExtentWriter: an ExtentWriter concrete subclass that writes
to another ExtentWriter.  BzipExtentWriter bzip2 decompresses all data
passed through.

[Daniel Erat, 2010-01-23 02:13:50]

I haven't looked at the tests in much detail.

http://codereview.chromium.org/551132/diff/1/2
File src/package_repo/package-list-dev.txt (right):

http://codereview.chromium.org/551132/diff/1/2#newcode63
src/package_repo/package-list-dev.txt:63: libbz2-dev
can you move this into a "Packages needed to build chromeos-updateengine" (or
whatever this package will be called) section?  that seems to be what we do with
most other packages.

http://codereview.chromium.org/551132/diff/1/4
File src/platform/update_engine/bzip_extent_writer.cc (right):

http://codereview.chromium.org/551132/diff/1/4#newcode10
src/platform/update_engine/bzip_extent_writer.cc:10: // what it's given in
Write. It passes the descompressed data to an underlying
delete this comment since it's already in the header

http://codereview.chromium.org/551132/diff/1/4#newcode26
src/platform/update_engine/bzip_extent_writer.cc:26: // use more memory
line this up with comment on previous line, or just shorten it to:

  // faster algo, more memory

http://codereview.chromium.org/551132/diff/1/5
File src/platform/update_engine/bzip_extent_writer.h (right):

http://codereview.chromium.org/551132/diff/1/5#newcode14
src/platform/update_engine/bzip_extent_writer.h:14: // what it's given in Write.
It passes the descompressed data to an underlying
s/descompressed/decompressed/

http://codereview.chromium.org/551132/diff/1/7
File src/platform/update_engine/extent_writer.cc (right):

http://codereview.chromium.org/551132/diff/1/7#newcode16
src/platform/update_engine/extent_writer.cc:16: bool WriteAll(int fd, const void
*buf, size_t count) {
wasn't this something that you'd already moved out into a utility file in v1? 
or was that just for reading?

http://codereview.chromium.org/551132/diff/1/8
File src/platform/update_engine/extent_writer.h (right):

http://codereview.chromium.org/551132/diff/1/8#newcode5
src/platform/update_engine/extent_writer.h:5: #ifndef
CHROMEOS_PLATFORM_UPDATE_ENGINE_EXTENT_WRITER_H__
doesn't need to happen in this CL, but i think that the convention for these is
now UPDATE_ENGINE_EXTENT_WRITER_

http://codereview.chromium.org/551132/diff/1/8#newcode16
src/platform/update_engine/extent_writer.h:16: namespace chromeos_update_engine
{
and i think that this should just be update_engine

http://codereview.chromium.org/551132/diff/1/8#newcode18
src/platform/update_engine/extent_writer.h:18: const uint64 kSparseHole =
kuint64max;
it's probably worthwhile describing this in a bit more detail (in the .proto
file?).  when an extent has this as its start block, it basically means that we
just drop those (num_extent * block_size) bytes on the floor instead of writing
them anywhere, right?

http://codereview.chromium.org/551132/diff/1/8#newcode25
src/platform/update_engine/extent_writer.h:25: virtual bool Init(int fd,
it might be nicer to hide this complexity from the caller -- you could pass
these parameters in through the c'tor and defer initialization until the first
time that Write() is called.  i'd probably move the data members that store the
parameters into ExtentWriter, too, instead of repeating them in each derived
class.

i'd also mention whether the writer is responsible for closing the fd or not --
judging from DirectExtentWriter(), it looks like it isn't.

http://codereview.chromium.org/551132/diff/1/8#newcode27
src/platform/update_engine/extent_writer.h:27: uint32 block_size) = 0;
should this be size_t, technically?  or perhaps that's just memory, and there's
something else for disks -- ssize_t?  (same goes for implementations)

http://codereview.chromium.org/551132/diff/1/8#newcode33
src/platform/update_engine/extent_writer.h:33: virtual bool End() = 0;
it'd be good to check that this has actually been invoked in the d'tor, probably
by adding an 'end_called_' data member to ExtentWriter, setting it in
ExtentWriter::End(), and adding a ExtentWriter::EndImpl() method that gets
implemented by derived classes and called by End().

http://codereview.chromium.org/551132/diff/1/8#newcode61
src/platform/update_engine/extent_writer.h:61: uint64 extent_bytes_written_;
size_t here too?

http://codereview.chromium.org/551132/diff/1/8#newcode64
src/platform/update_engine/extent_writer.h:64: std::vector<Extent>::size_type
next_extent_;
rename this to something like next_extent_index_ to make it clear from the name
that it's not an actual Extent.

http://codereview.chromium.org/551132/diff/1/8#newcode70
src/platform/update_engine/extent_writer.h:70: // to padd as needed.
s/padd/pad/

http://codereview.chromium.org/551132/diff/1/8#newcode93
src/platform/update_engine/extent_writer.h:93: std::vector<char>
zeros(write_size);
you can just pass (write_size, 0) and then skip the call to memset

http://codereview.chromium.org/551132/diff/1/8#newcode101
src/platform/update_engine/extent_writer.h:101: ExtentWriter* next_;  // The
underlying ExtentWriter.
'next_' is vague.  rename to something like 'real_extent_writer_'

[adlr, 2010-02-02 22:53:12]

http://codereview.chromium.org/551132/diff/1/2
File src/package_repo/package-list-dev.txt (right):

http://codereview.chromium.org/551132/diff/1/2#newcode63
src/package_repo/package-list-dev.txt:63: libbz2-dev
On 2010/01/23 02:13:50, Daniel Erat wrote:
> can you move this into a "Packages needed to build chromeos-updateengine" (or
> whatever this package will be called) section?  that seems to be what we do
with
> most other packages.

I'm just taking this out for now. Will add it in a future CL.

http://codereview.chromium.org/551132/diff/1/4
File src/platform/update_engine/bzip_extent_writer.cc (right):

http://codereview.chromium.org/551132/diff/1/4#newcode10
src/platform/update_engine/bzip_extent_writer.cc:10: // what it's given in
Write. It passes the descompressed data to an underlying
On 2010/01/23 02:13:50, Daniel Erat wrote:
> delete this comment since it's already in the header

Done.

http://codereview.chromium.org/551132/diff/1/4#newcode26
src/platform/update_engine/bzip_extent_writer.cc:26: // use more memory
On 2010/01/23 02:13:50, Daniel Erat wrote:
> line this up with comment on previous line, or just shorten it to:
> 
>   // faster algo, more memory

Done.

http://codereview.chromium.org/551132/diff/1/5
File src/platform/update_engine/bzip_extent_writer.h (right):

http://codereview.chromium.org/551132/diff/1/5#newcode14
src/platform/update_engine/bzip_extent_writer.h:14: // what it's given in Write.
It passes the descompressed data to an underlying
On 2010/01/23 02:13:50, Daniel Erat wrote:
> s/descompressed/decompressed/

Done.

http://codereview.chromium.org/551132/diff/1/7
File src/platform/update_engine/extent_writer.cc (right):

http://codereview.chromium.org/551132/diff/1/7#newcode16
src/platform/update_engine/extent_writer.cc:16: bool WriteAll(int fd, const void
*buf, size_t count) {
On 2010/01/23 02:13:50, Daniel Erat wrote:
> wasn't this something that you'd already moved out into a utility file in v1? 
> or was that just for reading?

The functionality exists in DirectFileWriter, but this doesn't use that. In a
future CL I move it into utils; I could move that up into this CL

http://codereview.chromium.org/551132/diff/1/8
File src/platform/update_engine/extent_writer.h (right):

http://codereview.chromium.org/551132/diff/1/8#newcode5
src/platform/update_engine/extent_writer.h:5: #ifndef
CHROMEOS_PLATFORM_UPDATE_ENGINE_EXTENT_WRITER_H__
On 2010/01/23 02:13:50, Daniel Erat wrote:
> doesn't need to happen in this CL, but i think that the convention for these
is
> now UPDATE_ENGINE_EXTENT_WRITER_

Yeah, let's keep this in a future CL. I'm keeping a TODO list of things for
future CLs

http://codereview.chromium.org/551132/diff/1/8#newcode16
src/platform/update_engine/extent_writer.h:16: namespace chromeos_update_engine
{
On 2010/01/23 02:13:50, Daniel Erat wrote:
> and i think that this should just be update_engine

Again, let's keep this for a future CL since it goes across many files

http://codereview.chromium.org/551132/diff/1/8#newcode18
src/platform/update_engine/extent_writer.h:18: const uint64 kSparseHole =
kuint64max;
On 2010/01/23 02:13:50, Daniel Erat wrote:
> it's probably worthwhile describing this in a bit more detail (in the .proto
> file?).  when an extent has this as its start block, it basically means that
we
> just drop those (num_extent * block_size) bytes on the floor instead of
writing
> them anywhere, right?

yes; documented.

http://codereview.chromium.org/551132/diff/1/8#newcode25
src/platform/update_engine/extent_writer.h:25: virtual bool Init(int fd,
On 2010/01/23 02:13:50, Daniel Erat wrote:
> it might be nicer to hide this complexity from the caller -- you could pass
> these parameters in through the c'tor and defer initialization until the first
> time that Write() is called. 

Sometimes I find it useful to separate creation of an extent writer and usage,
but putting the args in the ctor from Init requires joining them. I figured args
to the ctor would be specifically for the kind of extent writer created.

> i'd probably move the data members that store the
> parameters into ExtentWriter, too, instead of repeating them in each derived
> class.

I think only one extent writer (DirectExtentWriter) stores a copy of those args.

> 
> i'd also mention whether the writer is responsible for closing the fd or not
--
> judging from DirectExtentWriter(), it looks like it isn't.

It's up to the caller to close, documented.

http://codereview.chromium.org/551132/diff/1/8#newcode27
src/platform/update_engine/extent_writer.h:27: uint32 block_size) = 0;
On 2010/01/23 02:13:50, Daniel Erat wrote:
> should this be size_t, technically?  or perhaps that's just memory, and
there's
> something else for disks -- ssize_t?  (same goes for implementations)

I never really thought of size_t as for memory and ssize_t for disks. I figured
they were more or less for the same kinds of things, but the extra 's' in
ssize_t stood for "signed". Tho, I had no formal education in this area.

Anyway, sounds reasonable. Changed to size_t.

http://codereview.chromium.org/551132/diff/1/8#newcode33
src/platform/update_engine/extent_writer.h:33: virtual bool End() = 0;
On 2010/01/23 02:13:50, Daniel Erat wrote:
> it'd be good to check that this has actually been invoked in the d'tor,
probably
> by adding an 'end_called_' data member to ExtentWriter, setting it in
> ExtentWriter::End(), and adding a ExtentWriter::EndImpl() method that gets
> implemented by derived classes and called by End().

Done.

http://codereview.chromium.org/551132/diff/1/8#newcode61
src/platform/update_engine/extent_writer.h:61: uint64 extent_bytes_written_;
On 2010/01/23 02:13:50, Daniel Erat wrote:
> size_t here too?

size_t is 32-bit for us, but I want the number of bytes written to be able to
exceed 4 gigs, so keeping uint64

http://codereview.chromium.org/551132/diff/1/8#newcode64
src/platform/update_engine/extent_writer.h:64: std::vector<Extent>::size_type
next_extent_;
On 2010/01/23 02:13:50, Daniel Erat wrote:
> rename this to something like next_extent_index_ to make it clear from the
name
> that it's not an actual Extent.

Done.

http://codereview.chromium.org/551132/diff/1/8#newcode70
src/platform/update_engine/extent_writer.h:70: // to padd as needed.
On 2010/01/23 02:13:50, Daniel Erat wrote:
> s/padd/pad/

Done.

http://codereview.chromium.org/551132/diff/1/8#newcode93
src/platform/update_engine/extent_writer.h:93: std::vector<char>
zeros(write_size);
On 2010/01/23 02:13:50, Daniel Erat wrote:
> you can just pass (write_size, 0) and then skip the call to memset

Done.

http://codereview.chromium.org/551132/diff/1/8#newcode101
src/platform/update_engine/extent_writer.h:101: ExtentWriter* next_;  // The
underlying ExtentWriter.
On 2010/01/23 02:13:50, Daniel Erat wrote:
> 'next_' is vague.  rename to something like 'real_extent_writer_'

I don't know what makes on extent writer more real than another. next_ could
(although it's contrived) point to another ZeroPadExtentWriter. I changed it to
underlying_extent_writer_

[Daniel Erat, 2010-02-03 02:12:24]

LGTM

http://codereview.chromium.org/551132/diff/5001/5003
File src/platform/update_engine/bzip_extent_writer.cc (right):

http://codereview.chromium.org/551132/diff/5001/5003#newcode61
src/platform/update_engine/bzip_extent_writer.cc:61:
new_input_buffer.insert(new_input_buffer.end(),
i think you can just pass the two iterators to the c'tor and skip calling
insert()

http://codereview.chromium.org/551132/diff/5001/5004
File src/platform/update_engine/bzip_extent_writer.h (right):

http://codereview.chromium.org/551132/diff/5001/5004#newcode13
src/platform/update_engine/bzip_extent_writer.h:13: // BzipExtentWriter is a
concrete ExtentWriter subclass that bzip decompresses
s/bzip decompresses/bzip-decompresses/

(just 'cause i misparse this every time i read it otherwise)

http://codereview.chromium.org/551132/diff/5001/5005
File src/platform/update_engine/bzip_extent_writer_unittest.cc (right):

http://codereview.chromium.org/551132/diff/5001/5005#newcode72
src/platform/update_engine/bzip_extent_writer_unittest.cc:72: ssize_t rc =
pread(fd(), buf, sizeof(buf) - 1, 0);
rename 'rc' to something like 'bytes_read'

http://codereview.chromium.org/551132/diff/5001/5008
File src/platform/update_engine/extent_writer_unittest.cc (right):

http://codereview.chromium.org/551132/diff/5001/5008#newcode42
src/platform/update_engine/extent_writer_unittest.cc:42: void
WriteAlignedExtents(size_t chunk_size, size_t first_chunk_size);
add brief comments describing what these methods do

http://codereview.chromium.org/551132/diff/5001/5008#newcode247
src/platform/update_engine/extent_writer_unittest.cc:247:
//EXPECT_EQ(expected_data[0], resultant_data[0]);
why are these commented out?

[adlr, 2010-02-04 22:25:37]

fixed and submitted. thanks!

http://codereview.chromium.org/551132/diff/5001/5003
File src/platform/update_engine/bzip_extent_writer.cc (right):

http://codereview.chromium.org/551132/diff/5001/5003#newcode61
src/platform/update_engine/bzip_extent_writer.cc:61:
new_input_buffer.insert(new_input_buffer.end(),
On 2010/02/03 02:12:24, Daniel Erat wrote:
> i think you can just pass the two iterators to the c'tor and skip calling
> insert()

Done.

http://codereview.chromium.org/551132/diff/5001/5004
File src/platform/update_engine/bzip_extent_writer.h (right):

http://codereview.chromium.org/551132/diff/5001/5004#newcode13
src/platform/update_engine/bzip_extent_writer.h:13: // BzipExtentWriter is a
concrete ExtentWriter subclass that bzip decompresses
On 2010/02/03 02:12:24, Daniel Erat wrote:
> s/bzip decompresses/bzip-decompresses/
> 
> (just 'cause i misparse this every time i read it otherwise)

Done.

http://codereview.chromium.org/551132/diff/5001/5005
File src/platform/update_engine/bzip_extent_writer_unittest.cc (right):

http://codereview.chromium.org/551132/diff/5001/5005#newcode72
src/platform/update_engine/bzip_extent_writer_unittest.cc:72: ssize_t rc =
pread(fd(), buf, sizeof(buf) - 1, 0);
On 2010/02/03 02:12:24, Daniel Erat wrote:
> rename 'rc' to something like 'bytes_read'

Done.

http://codereview.chromium.org/551132/diff/5001/5008
File src/platform/update_engine/extent_writer_unittest.cc (right):

http://codereview.chromium.org/551132/diff/5001/5008#newcode42
src/platform/update_engine/extent_writer_unittest.cc:42: void
WriteAlignedExtents(size_t chunk_size, size_t first_chunk_size);
On 2010/02/03 02:12:24, Daniel Erat wrote:
> add brief comments describing what these methods do

Done.

http://codereview.chromium.org/551132/diff/5001/5008#newcode247
src/platform/update_engine/extent_writer_unittest.cc:247:
//EXPECT_EQ(expected_data[0], resultant_data[0]);
On 2010/02/03 02:12:24, Daniel Erat wrote:
> why are these commented out?

Oops. they were helpful when the test was failing, but redundant when it passes.
removed these lines.