AU: Delta Diff Generator

AU: Delta Diff Generator

Adds a class that can take two root filesystem image and generate a
delta between them. Currently it's not very well tested, but this will
improve once the diff applicator is written.

Also, an executable to run the generator.

Other changes:
- Stop leaking loop devices in unittests
- extent mapper: support sparse files, ability to get FS block size
- AppendBlockToExtents support sparse files
- subprocess more verbose on errors
- add WriteAll to utils (WriteAll avoids short-write() returns)
- mkstemp wrapper for ease of use
- VectorIndexOf, finds index of an element in a vector

[adlr, 2010-03-12 01:42:37]

It's not as bad as it looks. delta_diff_generator* is the meat of it, the other
files are pretty minor, a fair number of which are just deleting the
obsolete_logging header.

-andrew

[Daniel Erat, 2010-03-16 21:55:30]

just giving you a few early comments so i can slack off a bit more

http://codereview.chromium.org/891002/diff/3001/4002
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/891002/diff/3001/4002#newcode73
src/platform/update_engine/delta_diff_generator.cc:73: LOG(INFO) << "here";
you probably didn't mean to keep all these, right?  (goes for other LOG(INFO)s
in this file too)

http://codereview.chromium.org/891002/diff/3001/4002#newcode85
src/platform/update_engine/delta_diff_generator.cc:85: bool
AddExtentsToBlocks(const DeltaArchiveManifest_InstallOperation& operation,
it'd be helpful to preface functions like these with a high-level comment
explaining what they do, since there's no description in the header.

also, these should all be static

http://codereview.chromium.org/891002/diff/3001/4002#newcode330
src/platform/update_engine/delta_diff_generator.cc:330:
static_cast<vector<char>::size_type>(it->num_blocks() - blocks_read));
wrap long line

http://codereview.chromium.org/891002/diff/3001/4002#newcode585
src/platform/update_engine/delta_diff_generator.cc:585: LOG(INFO) << "using " <<
graph_utils::EdgeWeight(*graph, *it) << " scratch blocks ("
wrap long line

http://codereview.chromium.org/891002/diff/3001/4002#newcode603
src/platform/update_engine/delta_diff_generator.cc:603:
graph->back().op.set_src_length(graph_utils::EdgeWeight(*graph, *it) *
kBlockSize);
wrap long line

http://codereview.chromium.org/891002/diff/3001/4002#newcode853
src/platform/update_engine/delta_diff_generator.cc:853: LOG(INFO) << "Writing
final delta file protobuf... " << serialized_manifest.size();
wrap long line

http://codereview.chromium.org/891002/diff/3001/4003
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/891002/diff/3001/4003#newcode23
src/platform/update_engine/delta_diff_generator.h:23: struct Block {
If you don't need to use this anywhere outside of DeltaDiffGenerator, make it be
an internal class of it?

http://codereview.chromium.org/891002/diff/3001/4003#newcode25
src/platform/update_engine/delta_diff_generator.h:25: Vertex::Index reader;
describe what these are, or give them more descriptive names.  a name like
'reader' makes me think that it's something that actually reads, like a
RecordReader.

http://codereview.chromium.org/891002/diff/3001/4003#newcode32
src/platform/update_engine/delta_diff_generator.h:32: static bool
GenerateDeltaUpdateFile(const std::string& old_root,
describe what these parameters are (filenames, empty directories, etc.)

http://codereview.chromium.org/891002/diff/3001/4003#newcode40
src/platform/update_engine/delta_diff_generator.h:40: // necessary data in
out_data and fill in out_op.
s/fill/fills

http://codereview.chromium.org/891002/diff/3001/4003#newcode46
src/platform/update_engine/delta_diff_generator.h:46: static bool
ReadFileToDiff(const std::string& old_file,
if 'old_file' and 'new_file' are paths (as opposed to data), it'd be better to
name them 'old_filename' and 'new_filename'

http://codereview.chromium.org/891002/diff/3001/4003#newcode60
src/platform/update_engine/delta_diff_generator.h:60: const std::vector<Extent>&
remove_blocks,
maybe name these 'remove_extent' and 'replace_extent' instead... it's kind of
confusing how CutEdges() has a 'blocks' parameter too, but it's a Block vector
instead

[adlr, 2010-03-17 20:47:41]

ready for another look. thanks!

http://codereview.chromium.org/891002/diff/3001/4002
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/891002/diff/3001/4002#newcode73
src/platform/update_engine/delta_diff_generator.cc:73: LOG(INFO) << "here";
On 2010/03/16 21:55:30, Daniel Erat wrote:
> you probably didn't mean to keep all these, right?  (goes for other LOG(INFO)s
> in this file too)

right; removed

http://codereview.chromium.org/891002/diff/3001/4002#newcode85
src/platform/update_engine/delta_diff_generator.cc:85: bool
AddExtentsToBlocks(const DeltaArchiveManifest_InstallOperation& operation,
On 2010/03/16 21:55:30, Daniel Erat wrote:
> it'd be helpful to preface functions like these with a high-level comment
> explaining what they do, since there's no description in the header.
> 
> also, these should all be static

they are within namespace {}. I've added comments

http://codereview.chromium.org/891002/diff/3001/4002#newcode330
src/platform/update_engine/delta_diff_generator.cc:330:
static_cast<vector<char>::size_type>(it->num_blocks() - blocks_read));
On 2010/03/16 21:55:30, Daniel Erat wrote:
> wrap long line

Done.

http://codereview.chromium.org/891002/diff/3001/4002#newcode585
src/platform/update_engine/delta_diff_generator.cc:585: LOG(INFO) << "using " <<
graph_utils::EdgeWeight(*graph, *it) << " scratch blocks ("
On 2010/03/16 21:55:30, Daniel Erat wrote:
> wrap long line

Done.

http://codereview.chromium.org/891002/diff/3001/4002#newcode603
src/platform/update_engine/delta_diff_generator.cc:603:
graph->back().op.set_src_length(graph_utils::EdgeWeight(*graph, *it) *
kBlockSize);
On 2010/03/16 21:55:30, Daniel Erat wrote:
> wrap long line

Done.

http://codereview.chromium.org/891002/diff/3001/4002#newcode853
src/platform/update_engine/delta_diff_generator.cc:853: LOG(INFO) << "Writing
final delta file protobuf... " << serialized_manifest.size();
On 2010/03/16 21:55:30, Daniel Erat wrote:
> wrap long line

Done.

http://codereview.chromium.org/891002/diff/3001/4003
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/891002/diff/3001/4003#newcode23
src/platform/update_engine/delta_diff_generator.h:23: struct Block {
On 2010/03/16 21:55:30, Daniel Erat wrote:
> If you don't need to use this anywhere outside of DeltaDiffGenerator, make it
be
> an internal class of it?

Done.

http://codereview.chromium.org/891002/diff/3001/4003#newcode25
src/platform/update_engine/delta_diff_generator.h:25: Vertex::Index reader;
On 2010/03/16 21:55:30, Daniel Erat wrote:
> describe what these are, or give them more descriptive names.  a name like
> 'reader' makes me think that it's something that actually reads, like a
> RecordReader.

Done.

http://codereview.chromium.org/891002/diff/3001/4003#newcode32
src/platform/update_engine/delta_diff_generator.h:32: static bool
GenerateDeltaUpdateFile(const std::string& old_root,
On 2010/03/16 21:55:30, Daniel Erat wrote:
> describe what these parameters are (filenames, empty directories, etc.)

Done.

http://codereview.chromium.org/891002/diff/3001/4003#newcode40
src/platform/update_engine/delta_diff_generator.h:40: // necessary data in
out_data and fill in out_op.
On 2010/03/16 21:55:30, Daniel Erat wrote:
> s/fill/fills

Done.

http://codereview.chromium.org/891002/diff/3001/4003#newcode46
src/platform/update_engine/delta_diff_generator.h:46: static bool
ReadFileToDiff(const std::string& old_file,
On 2010/03/16 21:55:30, Daniel Erat wrote:
> if 'old_file' and 'new_file' are paths (as opposed to data), it'd be better to
> name them 'old_filename' and 'new_filename'

Done.

http://codereview.chromium.org/891002/diff/3001/4003#newcode60
src/platform/update_engine/delta_diff_generator.h:60: const std::vector<Extent>&
remove_blocks,
On 2010/03/16 21:55:30, Daniel Erat wrote:
> maybe name these 'remove_extent' and 'replace_extent' instead... it's kind of
> confusing how CutEdges() has a 'blocks' parameter too, but it's a Block vector
> instead

Done.

[Daniel Erat, 2010-03-18 01:00:27]

http://codereview.chromium.org/891002/diff/6002/9002
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/891002/diff/6002/9002#newcode90
src/platform/update_engine/delta_diff_generator.cc:90: bool
AddExtentsToBlocks(const DeltaArchiveManifest_InstallOperation& operation,
this name seems a bit ambiguous to me -- isn't an extent just a series of
contiguous blocks?

something like "UpdateReadersAndWritersForBlocks()" seems like it'd be more
precise (or maybe you could refactor this as a shorter function that gets called
once to update the readers and a second time to update the writers -- it looks
like the loops are essentially identical apart from what they're iterating over
and the field that they're setting)

http://codereview.chromium.org/891002/diff/6002/9002#newcode181
src/platform/update_engine/delta_diff_generator.cc:181: // determine the best
way to compress it (REPLACE, REPLACE_BZ, COPY, BSDIFF),
s/determine/determines/

http://codereview.chromium.org/891002/diff/6002/9002#newcode219
src/platform/update_engine/delta_diff_generator.cc:219: // In the future, we
could consider returning all scratch blocks,
s/In the future, we could/TODO:/

http://codereview.chromium.org/891002/diff/6002/9002#newcode227
src/platform/update_engine/delta_diff_generator.cc:227: // In the future, we
could return blocks that are written by
s/In the future, we could/TODO:/

http://codereview.chromium.org/891002/diff/6002/9002#newcode256
src/platform/update_engine/delta_diff_generator.cc:256: : extents_(extents),
extent_index_(0), extent_blocks_allocated_(0) {}
nit: one member per line if they don't fit on the same line as the c'tor
signature

http://codereview.chromium.org/891002/diff/6002/9002#newcode299
src/platform/update_engine/delta_diff_generator.cc:299:
CHECK_EQ(sizeof(temp_file_path), sizeof(kTempFileTemplate));
i don't understand this check or the one before it.  why does BsdiffFiles() use
vector<char> instead?  could you create a helper function for this?

http://codereview.chromium.org/891002/diff/6002/9002#newcode306
src/platform/update_engine/delta_diff_generator.cc:306: FILE* file =
fopen(temp_file_path, "w");
you should check 'file' here

http://codereview.chromium.org/891002/diff/6002/9002#newcode326
src/platform/update_engine/delta_diff_generator.cc:326: vector<char> buf(1024 *
kBlockSize);
where does the 1024 come from?

http://codereview.chromium.org/891002/diff/6002/9002#newcode330
src/platform/update_engine/delta_diff_generator.cc:330: for
(vector<Extent>::const_iterator it = extents.begin();
this loop deserves a comment describing what its body is doing

http://codereview.chromium.org/891002/diff/6002/9002#newcode336
src/platform/update_engine/delta_diff_generator.cc:336:
static_cast<vector<char>::size_type>(it->num_blocks() - blocks_read));
wrap long line

http://codereview.chromium.org/891002/diff/6002/9003
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/891002/diff/6002/9003#newcode40
src/platform/update_engine/delta_diff_generator.h:40: // This is the only
function that external users of the class should call.
Can you make everything else private, then?

http://codereview.chromium.org/891002/diff/6002/9003#newcode57
src/platform/update_engine/delta_diff_generator.h:57: // new_file must contain
at least one byte.
s/new_file/new_filename/

http://codereview.chromium.org/891002/diff/6002/9003#newcode65
src/platform/update_engine/delta_diff_generator.h:65: // 'remove_blocks' are
replaced with blocks from 'replace_blocks'.
switch the "blocks" in this comment to "extents"

http://codereview.chromium.org/891002/diff/6002/9003#newcode99
src/platform/update_engine/delta_diff_generator.h:99: // operation is in the
manifest. E.g. if manifest[0] has a data blob
maybe rephrase this as "... in the same order as the referencing install
operations in the manifest."

[adlr, 2010-03-19 01:13:43]

thanks for reviewing; i've made the changes.

have another look

http://codereview.chromium.org/891002/diff/6002/9002
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/891002/diff/6002/9002#newcode90
src/platform/update_engine/delta_diff_generator.cc:90: bool
AddExtentsToBlocks(const DeltaArchiveManifest_InstallOperation& operation,
On 2010/03/18 01:00:27, Daniel Erat wrote:
> this name seems a bit ambiguous to me -- isn't an extent just a series of
> contiguous blocks?
> 
> something like "UpdateReadersAndWritersForBlocks()" seems like it'd be more
> precise (or maybe you could refactor this as a shorter function that gets
called
> once to update the readers and a second time to update the writers -- it looks
> like the loops are essentially identical apart from what they're iterating
over
> and the field that they're setting)

Renamed. Also removed the code duplication, which I think gave me my first
reason to ever use C++ pointer to member operators.

http://codereview.chromium.org/891002/diff/6002/9002#newcode181
src/platform/update_engine/delta_diff_generator.cc:181: // determine the best
way to compress it (REPLACE, REPLACE_BZ, COPY, BSDIFF),
On 2010/03/18 01:00:27, Daniel Erat wrote:
> s/determine/determines/

Done.

http://codereview.chromium.org/891002/diff/6002/9002#newcode219
src/platform/update_engine/delta_diff_generator.cc:219: // In the future, we
could consider returning all scratch blocks,
On 2010/03/18 01:00:27, Daniel Erat wrote:
> s/In the future, we could/TODO:/

Done.

http://codereview.chromium.org/891002/diff/6002/9002#newcode227
src/platform/update_engine/delta_diff_generator.cc:227: // In the future, we
could return blocks that are written by
On 2010/03/18 01:00:27, Daniel Erat wrote:
> s/In the future, we could/TODO:/

Done.

http://codereview.chromium.org/891002/diff/6002/9002#newcode256
src/platform/update_engine/delta_diff_generator.cc:256: : extents_(extents),
extent_index_(0), extent_blocks_allocated_(0) {}
On 2010/03/18 01:00:27, Daniel Erat wrote:
> nit: one member per line if they don't fit on the same line as the c'tor
> signature

Done.

http://codereview.chromium.org/891002/diff/6002/9002#newcode299
src/platform/update_engine/delta_diff_generator.cc:299:
CHECK_EQ(sizeof(temp_file_path), sizeof(kTempFileTemplate));
On 2010/03/18 01:00:27, Daniel Erat wrote:
> i don't understand this check or the one before it.  why does BsdiffFiles()
use
> vector<char> instead?  could you create a helper function for this?

sorry, some stupid sanity checks. I've replaced w/ my mkstemp helper function

http://codereview.chromium.org/891002/diff/6002/9002#newcode306
src/platform/update_engine/delta_diff_generator.cc:306: FILE* file =
fopen(temp_file_path, "w");
On 2010/03/18 01:00:27, Daniel Erat wrote:
> you should check 'file' here

Done.

http://codereview.chromium.org/891002/diff/6002/9002#newcode326
src/platform/update_engine/delta_diff_generator.cc:326: vector<char> buf(1024 *
kBlockSize);
On 2010/03/18 01:00:27, Daniel Erat wrote:
> where does the 1024 come from?

Done.

http://codereview.chromium.org/891002/diff/6002/9002#newcode330
src/platform/update_engine/delta_diff_generator.cc:330: for
(vector<Extent>::const_iterator it = extents.begin();
On 2010/03/18 01:00:27, Daniel Erat wrote:
> this loop deserves a comment describing what its body is doing

Done.

http://codereview.chromium.org/891002/diff/6002/9002#newcode336
src/platform/update_engine/delta_diff_generator.cc:336:
static_cast<vector<char>::size_type>(it->num_blocks() - blocks_read));
On 2010/03/18 01:00:27, Daniel Erat wrote:
> wrap long line

Done.

http://codereview.chromium.org/891002/diff/6002/9003
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/891002/diff/6002/9003#newcode40
src/platform/update_engine/delta_diff_generator.h:40: // This is the only
function that external users of the class should call.
On 2010/03/18 01:00:27, Daniel Erat wrote:
> Can you make everything else private, then?

Public for testing. Documented.

http://codereview.chromium.org/891002/diff/6002/9003#newcode57
src/platform/update_engine/delta_diff_generator.h:57: // new_file must contain
at least one byte.
On 2010/03/18 01:00:27, Daniel Erat wrote:
> s/new_file/new_filename/

Done.

http://codereview.chromium.org/891002/diff/6002/9003#newcode65
src/platform/update_engine/delta_diff_generator.h:65: // 'remove_blocks' are
replaced with blocks from 'replace_blocks'.
On 2010/03/18 01:00:27, Daniel Erat wrote:
> switch the "blocks" in this comment to "extents"

Done.

http://codereview.chromium.org/891002/diff/6002/9003#newcode99
src/platform/update_engine/delta_diff_generator.h:99: // operation is in the
manifest. E.g. if manifest[0] has a data blob
On 2010/03/18 01:00:27, Daniel Erat wrote:
> maybe rephrase this as "... in the same order as the referencing install
> operations in the manifest."

Done.

[Daniel Erat, 2010-03-28 16:19:41]

Sorry for taking so long on this.  LGTM after these (mostly stylistic) comments
are addressed.

http://codereview.chromium.org/891002/diff/13001/14002
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/891002/diff/13001/14002#newcode63
src/platform/update_engine/delta_diff_generator.cc:63: int fd =
mkstemp(&patch_file_path[0]);
use utils::MksTemp() here

http://codereview.chromium.org/891002/diff/13001/14002#newcode103
src/platform/update_engine/delta_diff_generator.cc:103: ( field == READER ?
operation.src_extents_size() :
nice refactoring!  can you switch this to:

  (file == READER) ? ...

?  i think that the style guide complains about space-padded parentheses (ditto
for other lines around here)

http://codereview.chromium.org/891002/diff/13001/14002#newcode712
src/platform/update_engine/delta_diff_generator.cc:712: memcpy(temp_file_path,
kTempFileTemplate, sizeof(temp_file_path));
you can replace all of this with utils::MksTemp() too, right?

http://codereview.chromium.org/891002/diff/13001/14002#newcode802
src/platform/update_engine/delta_diff_generator.cc:802: LOG(INFO) << "bad blob
offset! " << op.data_offset() << " != "
should this and the next LOG(INFO) be CHECK()s or DCHECK()s?  seems like you
might want to crash here instead of logging a message that people might miss and
writing a possibly-bad update image

http://codereview.chromium.org/891002/diff/13001/14002#newcode850
src/platform/update_engine/delta_diff_generator.cc:850: int blobs_fd =
open(temp_file_path, O_RDONLY, 0);
use a ScopedFdCloser here?  it looks like you leak the fd if you return from
within the for loop

http://codereview.chromium.org/891002/diff/13001/14003
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/891002/diff/13001/14003#newcode36
src/platform/update_engine/delta_diff_generator.h:36: Vertex::Index reader;
is it possible that a single block will be read by multiple vertices?

http://codereview.chromium.org/891002/diff/13001/14003#newcode53
src/platform/update_engine/delta_diff_generator.h:53: // Reads an old_file (if
it exists) and a new_file and determines
s/_/ /g (at least, old_file and new_file don't match the parameter names
anymore)

http://codereview.chromium.org/891002/diff/13001/14004
File src/platform/update_engine/delta_diff_generator_unittest.cc (right):

http://codereview.chromium.org/891002/diff/13001/14004#newcode253
src/platform/update_engine/delta_diff_generator_unittest.cc:253:
EXPECT_TRUE(cut_edges.end() != cut_edges.find(make_pair<Vertex::Index>(1, 0)));
wrap long line

http://codereview.chromium.org/891002/diff/13001/14005
File src/platform/update_engine/extent_mapper.cc (right):

http://codereview.chromium.org/891002/diff/13001/14005#newcode63
src/platform/update_engine/extent_mapper.cc:63: if (current.num_blocks() == 0) {
this stuff looks the same as graph_utils::AppendBlockToExtents().  can you make
another utility function that both of them call instead of duplicating the code?

http://codereview.chromium.org/891002/diff/13001/14006
File src/platform/update_engine/extent_mapper.h (right):

http://codereview.chromium.org/891002/diff/13001/14006#newcode17
src/platform/update_engine/extent_mapper.h:17: bool ExtentsForFileFibmap(const
std::string& path, std::vector<Extent>* out);
add a blank line between these and comments describing what they do

http://codereview.chromium.org/891002/diff/13001/14008
File src/platform/update_engine/extent_writer.cc (right):

http://codereview.chromium.org/891002/diff/13001/14008#newcode38
src/platform/update_engine/extent_writer.cc:38: utils::WriteAll(fd_, c_bytes +
bytes_written, bytes_to_write) >= 0);
get rid of the ">= 0" (utils::WriteAll() returns bool)

http://codereview.chromium.org/891002/diff/13001/14011
File src/platform/update_engine/generate_delta_main.cc (right):

http://codereview.chromium.org/891002/diff/13001/14011#newcode14
src/platform/update_engine/generate_delta_main.cc:14: #include
"base/command_line.h"
alphabetize (move above chromeos)

http://codereview.chromium.org/891002/diff/13001/14011#newcode32
src/platform/update_engine/generate_delta_main.cc:32: printf("usage: %s old_dir
new_dir old_image new_image out_file\n", argv0);
it'd be good to switch to using gflag for these at some point -- this command
line is impossible to decipher without looking at the usage string

http://codereview.chromium.org/891002/diff/13001/14013
File src/platform/update_engine/graph_utils.cc (right):

http://codereview.chromium.org/891002/diff/13001/14013#newcode6
src/platform/update_engine/graph_utils.cc:6: #include
"chromeos/obsolete_logging.h"
i don't think you need to include this here

http://codereview.chromium.org/891002/diff/13001/14013#newcode32
src/platform/update_engine/graph_utils.cc:32: // Extend sparse hold extent
s/hold/hole/

http://codereview.chromium.org/891002/diff/13001/14013#newcode36
src/platform/update_engine/graph_utils.cc:36: // Create new extent below outside
outer 'if'
should be either "below" or "outside" rather than both, i think

http://codereview.chromium.org/891002/diff/13001/14017
File src/platform/update_engine/topological_sort.cc (right):

http://codereview.chromium.org/891002/diff/13001/14017#newcode8
src/platform/update_engine/topological_sort.cc:8: #include
"chromeos/obsolete_logging.h"
you can get LOG(INFO) from base/logging.h instead of needing obsolete_logging.h
(i still have dreams of getting rid of libchromeos at some point)

http://codereview.chromium.org/891002/diff/13001/14017#newcode30
src/platform/update_engine/topological_sort.cc:30: LOG(INFO) <<
graph[node].file_name << " " << graph[node].op.type() << " " <<
graph[node].op.data_length();
wrap long line

http://codereview.chromium.org/891002/diff/13001/14018
File src/platform/update_engine/utils.cc (right):

http://codereview.chromium.org/891002/diff/13001/14018#newcode39
src/platform/update_engine/utils.cc:39: const char* c_buf =
reinterpret_cast<const char*>(buf);
not a huge deal, but this could just be static_cast instead, right?

http://codereview.chromium.org/891002/diff/13001/14018#newcode42
src/platform/update_engine/utils.cc:42: ssize_t rc = write(fd, c_buf +
bytes_written, count - bytes_written);
i didn't know about this until recently, but chrome defines a HANDLE_EINTR()
wrapper macro in base/eintr_wrapper.h that you should technically be putting
around many of these syscalls

http://codereview.chromium.org/891002/diff/13001/14018#newcode217
src/platform/update_engine/utils.cc:217: int* fd) {
DCHECK() that either 'filename' or 'fd' is non-NULL?

http://codereview.chromium.org/891002/diff/13001/14018#newcode218
src/platform/update_engine/utils.cc:218: vector<char>
buf(filename_template.size() + 1);
you do this in other places too, right?  worth writing a utility function for
it?

http://codereview.chromium.org/891002/diff/13001/14019
File src/platform/update_engine/utils.h (right):

http://codereview.chromium.org/891002/diff/13001/14019#newcode23
src/platform/update_engine/utils.h:23: // Returns true on success.
briefly describe what this does in the comment (i can guess)

http://codereview.chromium.org/891002/diff/13001/14019#newcode49
src/platform/update_engine/utils.h:49: // is not close()d and is return in the
out param 'fd'. However, if
s/return/returned/

http://codereview.chromium.org/891002/diff/13001/14019#newcode52
src/platform/update_engine/utils.h:52: // Returns true on success
add trailing period

http://codereview.chromium.org/891002/diff/13001/14019#newcode53
src/platform/update_engine/utils.h:53: bool MksTemp(const std::string&
filename_template,
s/MksTemp/MakeTempFile/ ?

http://codereview.chromium.org/891002/diff/13001/14019#newcode104
src/platform/update_engine/utils.h:104: typename std::vector<T>::size_type*
out_index) {
maybe replace this with:

std::vector<T>::const_iterator it =
    std::find(vect.begin(), vect.end(), value);
if (it == vect.end()) {
  return false;
} else {
  *out_index = it - vect.begin();
  return true;
}

and just to be picky, ordering 'vect' before 'value' in the parameter list seems
more common (and in line with how STL and libc order the parameters to their
find-something-in-a-container functions)

http://codereview.chromium.org/891002/diff/13001/14019#newcode194
src/platform/update_engine/utils.h:194: #define TEST_AND_RETURN_FALSE(_x)       
       \
what was the reason for this change?  these were all consistent before; now the
amount of spacing before the backlashes in these macros feels arbitrary

[adlr, 2010-03-30 03:32:07]

Not submitting b/c I have a couple questions for you to answer.

Thanks,

http://codereview.chromium.org/891002/diff/13001/14002
File src/platform/update_engine/delta_diff_generator.cc (right):

http://codereview.chromium.org/891002/diff/13001/14002#newcode63
src/platform/update_engine/delta_diff_generator.cc:63: int fd =
mkstemp(&patch_file_path[0]);
On 2010/03/28 16:19:41, Daniel Erat wrote:
> use utils::MksTemp() here

Done.

http://codereview.chromium.org/891002/diff/13001/14002#newcode103
src/platform/update_engine/delta_diff_generator.cc:103: ( field == READER ?
operation.src_extents_size() :
On 2010/03/28 16:19:41, Daniel Erat wrote:
> nice refactoring!  can you switch this to:
> 
>   (file == READER) ? ...
> 
> ?  i think that the style guide complains about space-padded parentheses
(ditto
> for other lines around here)

Done.

http://codereview.chromium.org/891002/diff/13001/14002#newcode712
src/platform/update_engine/delta_diff_generator.cc:712: memcpy(temp_file_path,
kTempFileTemplate, sizeof(temp_file_path));
On 2010/03/28 16:19:41, Daniel Erat wrote:
> you can replace all of this with utils::MksTemp() too, right?

Done.

http://codereview.chromium.org/891002/diff/13001/14002#newcode802
src/platform/update_engine/delta_diff_generator.cc:802: LOG(INFO) << "bad blob
offset! " << op.data_offset() << " != "
On 2010/03/28 16:19:41, Daniel Erat wrote:
> should this and the next LOG(INFO) be CHECK()s or DCHECK()s?  seems like you
> might want to crash here instead of logging a message that people might miss
and
> writing a possibly-bad update image

changed to LOG(FATAL)

http://codereview.chromium.org/891002/diff/13001/14002#newcode850
src/platform/update_engine/delta_diff_generator.cc:850: int blobs_fd =
open(temp_file_path, O_RDONLY, 0);
On 2010/03/28 16:19:41, Daniel Erat wrote:
> use a ScopedFdCloser here?  it looks like you leak the fd if you return from
> within the for loop

good catch. fixed

http://codereview.chromium.org/891002/diff/13001/14003
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/891002/diff/13001/14003#newcode36
src/platform/update_engine/delta_diff_generator.h:36: Vertex::Index reader;
On 2010/03/28 16:19:41, Daniel Erat wrote:
> is it possible that a single block will be read by multiple vertices?

Currently no, although in the future a different algorithm could do that.

http://codereview.chromium.org/891002/diff/13001/14003#newcode53
src/platform/update_engine/delta_diff_generator.h:53: // Reads an old_file (if
it exists) and a new_file and determines
On 2010/03/28 16:19:41, Daniel Erat wrote:
> s/_/ /g (at least, old_file and new_file don't match the parameter names
> anymore)

fixed so comment uses current param names

http://codereview.chromium.org/891002/diff/13001/14004
File src/platform/update_engine/delta_diff_generator_unittest.cc (right):

http://codereview.chromium.org/891002/diff/13001/14004#newcode253
src/platform/update_engine/delta_diff_generator_unittest.cc:253:
EXPECT_TRUE(cut_edges.end() != cut_edges.find(make_pair<Vertex::Index>(1, 0)));
On 2010/03/28 16:19:41, Daniel Erat wrote:
> wrap long line

Done.

http://codereview.chromium.org/891002/diff/13001/14005
File src/platform/update_engine/extent_mapper.cc (right):

http://codereview.chromium.org/891002/diff/13001/14005#newcode63
src/platform/update_engine/extent_mapper.cc:63: if (current.num_blocks() == 0) {
On 2010/03/28 16:19:41, Daniel Erat wrote:
> this stuff looks the same as graph_utils::AppendBlockToExtents().  can you
make
> another utility function that both of them call instead of duplicating the
code?

Done.

http://codereview.chromium.org/891002/diff/13001/14006
File src/platform/update_engine/extent_mapper.h (right):

http://codereview.chromium.org/891002/diff/13001/14006#newcode17
src/platform/update_engine/extent_mapper.h:17: bool ExtentsForFileFibmap(const
std::string& path, std::vector<Extent>* out);
On 2010/03/28 16:19:41, Daniel Erat wrote:
> add a blank line between these and comments describing what they do

Done.

http://codereview.chromium.org/891002/diff/13001/14008
File src/platform/update_engine/extent_writer.cc (right):

http://codereview.chromium.org/891002/diff/13001/14008#newcode38
src/platform/update_engine/extent_writer.cc:38: utils::WriteAll(fd_, c_bytes +
bytes_written, bytes_to_write) >= 0);
On 2010/03/28 16:19:41, Daniel Erat wrote:
> get rid of the ">= 0" (utils::WriteAll() returns bool)

Done.

http://codereview.chromium.org/891002/diff/13001/14011
File src/platform/update_engine/generate_delta_main.cc (right):

http://codereview.chromium.org/891002/diff/13001/14011#newcode14
src/platform/update_engine/generate_delta_main.cc:14: #include
"base/command_line.h"
On 2010/03/28 16:19:41, Daniel Erat wrote:
> alphabetize (move above chromeos)

Done.

http://codereview.chromium.org/891002/diff/13001/14011#newcode32
src/platform/update_engine/generate_delta_main.cc:32: printf("usage: %s old_dir
new_dir old_image new_image out_file\n", argv0);
On 2010/03/28 16:19:41, Daniel Erat wrote:
> it'd be good to switch to using gflag for these at some point -- this command
> line is impossible to decipher without looking at the usage string

Done.

http://codereview.chromium.org/891002/diff/13001/14013
File src/platform/update_engine/graph_utils.cc (right):

http://codereview.chromium.org/891002/diff/13001/14013#newcode6
src/platform/update_engine/graph_utils.cc:6: #include
"chromeos/obsolete_logging.h"
On 2010/03/28 16:19:41, Daniel Erat wrote:
> i don't think you need to include this here

Done.

http://codereview.chromium.org/891002/diff/13001/14013#newcode32
src/platform/update_engine/graph_utils.cc:32: // Extend sparse hold extent
On 2010/03/28 16:19:41, Daniel Erat wrote:
> s/hold/hole/

Done.

http://codereview.chromium.org/891002/diff/13001/14013#newcode36
src/platform/update_engine/graph_utils.cc:36: // Create new extent below outside
outer 'if'
On 2010/03/28 16:19:41, Daniel Erat wrote:
> should be either "below" or "outside" rather than both, i think

Done.

http://codereview.chromium.org/891002/diff/13001/14017
File src/platform/update_engine/topological_sort.cc (right):

http://codereview.chromium.org/891002/diff/13001/14017#newcode8
src/platform/update_engine/topological_sort.cc:8: #include
"chromeos/obsolete_logging.h"
On 2010/03/28 16:19:41, Daniel Erat wrote:
> you can get LOG(INFO) from base/logging.h instead of needing
obsolete_logging.h
> (i still have dreams of getting rid of libchromeos at some point)

Done.

http://codereview.chromium.org/891002/diff/13001/14017#newcode30
src/platform/update_engine/topological_sort.cc:30: LOG(INFO) <<
graph[node].file_name << " " << graph[node].op.type() << " " <<
graph[node].op.data_length();
On 2010/03/28 16:19:41, Daniel Erat wrote:
> wrap long line

Done.

http://codereview.chromium.org/891002/diff/13001/14018
File src/platform/update_engine/utils.cc (right):

http://codereview.chromium.org/891002/diff/13001/14018#newcode39
src/platform/update_engine/utils.cc:39: const char* c_buf =
reinterpret_cast<const char*>(buf);
On 2010/03/28 16:19:41, Daniel Erat wrote:
> not a huge deal, but this could just be static_cast instead, right?

ah yes. didn't know you could do that

http://codereview.chromium.org/891002/diff/13001/14018#newcode42
src/platform/update_engine/utils.cc:42: ssize_t rc = write(fd, c_buf +
bytes_written, count - bytes_written);
On 2010/03/28 16:19:41, Daniel Erat wrote:
> i didn't know about this until recently, but chrome defines a HANDLE_EINTR()
> wrapper macro in base/eintr_wrapper.h that you should technically be putting
> around many of these syscalls

How about I put this in a future CL? It's on the list...

http://codereview.chromium.org/891002/diff/13001/14018#newcode217
src/platform/update_engine/utils.cc:217: int* fd) {
On 2010/03/28 16:19:41, Daniel Erat wrote:
> DCHECK() that either 'filename' or 'fd' is non-NULL?

Done.

http://codereview.chromium.org/891002/diff/13001/14018#newcode218
src/platform/update_engine/utils.cc:218: vector<char>
buf(filename_template.size() + 1);
On 2010/03/28 16:19:41, Daniel Erat wrote:
> you do this in other places too, right?  worth writing a utility function for
> it?

what are you referring to? if you mean calling mkstemp, this is the utility
function

http://codereview.chromium.org/891002/diff/13001/14019
File src/platform/update_engine/utils.h (right):

http://codereview.chromium.org/891002/diff/13001/14019#newcode23
src/platform/update_engine/utils.h:23: // Returns true on success.
On 2010/03/28 16:19:41, Daniel Erat wrote:
> briefly describe what this does in the comment (i can guess)

Done.

http://codereview.chromium.org/891002/diff/13001/14019#newcode49
src/platform/update_engine/utils.h:49: // is not close()d and is return in the
out param 'fd'. However, if
On 2010/03/28 16:19:41, Daniel Erat wrote:
> s/return/returned/

Done.

http://codereview.chromium.org/891002/diff/13001/14019#newcode52
src/platform/update_engine/utils.h:52: // Returns true on success
On 2010/03/28 16:19:41, Daniel Erat wrote:
> add trailing period

Done.

http://codereview.chromium.org/891002/diff/13001/14019#newcode53
src/platform/update_engine/utils.h:53: bool MksTemp(const std::string&
filename_template,
On 2010/03/28 16:19:41, Daniel Erat wrote:
> s/MksTemp/MakeTempFile/ ?

Done.

http://codereview.chromium.org/891002/diff/13001/14019#newcode104
src/platform/update_engine/utils.h:104: typename std::vector<T>::size_type*
out_index) {
On 2010/03/28 16:19:41, Daniel Erat wrote:
> maybe replace this with:
> 
> std::vector<T>::const_iterator it =
>     std::find(vect.begin(), vect.end(), value);
> if (it == vect.end()) {
>   return false;
> } else {
>   *out_index = it - vect.begin();
>   return true;
> }
> 
> and just to be picky, ordering 'vect' before 'value' in the parameter list
seems
> more common (and in line with how STL and libc order the parameters to their
> find-something-in-a-container functions)

Done.

http://codereview.chromium.org/891002/diff/13001/14019#newcode194
src/platform/update_engine/utils.h:194: #define TEST_AND_RETURN_FALSE(_x)       
       \
On 2010/03/28 16:19:41, Daniel Erat wrote:
> what was the reason for this change?  these were all consistent before; now
the
> amount of spacing before the backlashes in these macros feels arbitrary

Sorry, my editor is changing them automatically. Fixed

[Daniel Erat, 2010-03-30 03:48:52]

http://codereview.chromium.org/891002/diff/13001/14018
File src/platform/update_engine/utils.cc (right):

http://codereview.chromium.org/891002/diff/13001/14018#newcode42
src/platform/update_engine/utils.cc:42: ssize_t rc = write(fd, c_buf +
bytes_written, count - bytes_written);
On 2010/03/30 03:32:07, adlr wrote:
> On 2010/03/28 16:19:41, Daniel Erat wrote:
> > i didn't know about this until recently, but chrome defines a HANDLE_EINTR()
> > wrapper macro in base/eintr_wrapper.h that you should technically be putting
> > around many of these syscalls
> 
> How about I put this in a future CL? It's on the list...

Sure, sounds good.

http://codereview.chromium.org/891002/diff/13001/14018#newcode218
src/platform/update_engine/utils.cc:218: vector<char>
buf(filename_template.size() + 1);
On 2010/03/30 03:32:07, adlr wrote:
> On 2010/03/28 16:19:41, Daniel Erat wrote:
> > you do this in other places too, right?  worth writing a utility function
for
> > it?
> 
> what are you referring to? if you mean calling mkstemp, this is the utility
> function

Sorry for not being more specific: copying std::string into std::vector<char>
and null-terminating it.