Add support for bsdiff of file system metadata blocks

Add support for bsdiff of file system metadata blocks

BUG=chromium-os:10188
TEST=Unit test, build delta update, apply to Mario and make sure it
boots with new version

Change-Id: I37b3fcc3c0e65e063cd95b0b3c9a5cd2261c98c9

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=5c7d975

[thieule, 2010-12-09 23:55:18]

Modify delta_generator to be aware of the file system metadata.  This CL alone
does not buy a huge savings.  Another CL patches build_image to preserve the
on-disk file layout between builds.  By preserving the on-disk file layout, it
minimizes the amount of metadata changes.

Here are some statistics comparing the current approach and this approach.  I
ran build_packages once, then build_image is run once for each approach. 
Basically, reusing the existing binaries to build two different images.  The
reduction in metadata overhead is an order of magnitude lower.

Current approach that where the file layout is not preserved and delta_generator
is not metadata aware:

  0.00%          0 MOVE       /bin/attr
  0.00%          0 MOVE       /bin/basename
  0.00%          0 MOVE       /bin/bash
  ...
  0.00%          0 MOVE       /usr/share/zoneinfo/right/WET
  0.00%          0 MOVE       /usr/share/zoneinfo/right/Zulu
  0.00%          0 MOVE       /usr/share/zoneinfo/zone.tab
  0.02%        139 BSDIFF     /etc/env.d/03opengl
  0.02%        139 BSDIFF     /etc/ld.so.conf
  0.02%        146 BSDIFF    
/usr/share/cache/fontconfig/16e0d25fe1bfa824c1fb282d406f28c3-x86.cache-2
  0.02%        147 BSDIFF    
/usr/share/cache/fontconfig/3830d5c3ddfd5cd38a049b759396e72e-x86.cache-2
  0.02%        148 BSDIFF    
/usr/share/cache/fontconfig/12513961c6e7090f8648812f9eaf65d6-x86.cache-2
  0.02%        151 BSDIFF     /etc/lsb-release
  0.02%        151 BSDIFF    
/usr/share/cache/fontconfig/1edfe47e05720ecccec11c0ae49cefdc-x86.cache-2
  0.02%        151 BSDIFF    
/usr/share/cache/fontconfig/9f60835a5eaa9f17b061039c787d35ba-x86.cache-2
  0.02%        152 BSDIFF    
/usr/share/cache/fontconfig/580bda594161061e6ef4cc8a4db09662-x86.cache-2
  0.02%        152 BSDIFF    
/usr/share/cache/fontconfig/a595ca43be94766bbfe429f07fba82d0-x86.cache-2
  0.02%        153 BSDIFF     /boot/efi/boot/bootx64.efi
  0.02%        153 BSDIFF    
/usr/share/cache/fontconfig/12b26b760a24f8b4feb03ad48a333a72-x86.cache-2
  0.02%        153 BSDIFF    
/usr/share/cache/fontconfig/aaac3452274625c4167ca2eab9cb5e70-x86.cache-2
  0.02%        173 BSDIFF     /boot/extlinux.sys
  0.06%        398 BSDIFF     /usr/lib/libsoftokn3.chk
  0.06%        402 BSDIFF     /usr/lib/libnssdbm3.chk
  0.06%        403 BSDIFF     /usr/lib/libfreebl3.chk
  0.38%       2646 BSDIFF     <kernel-operation-0>
 45.81%     317085 -          <manifest-metadata>
 53.33%     369075 REPLACE_BZ <rootfs-non-file-data>
100.00%     692117            <total>


The new approach that preserves the file layout and delta_generator is metadata
aware:


  0.48%        143 BSDIFF     /etc/env.d/03opengl
  0.48%        143 BSDIFF     /etc/ld.so.conf
  0.49%        146 BSDIFF    
/usr/share/cache/fontconfig/3830d5c3ddfd5cd38a049b759396e72e-x86.cache-2
  0.49%        147 BSDIFF    
/usr/share/cache/fontconfig/16e0d25fe1bfa824c1fb282d406f28c3-x86.cache-2
  0.50%        148 BSDIFF    
/usr/share/cache/fontconfig/12513961c6e7090f8648812f9eaf65d6-x86.cache-2
  0.51%        152 BSDIFF    
/usr/share/cache/fontconfig/1edfe47e05720ecccec11c0ae49cefdc-x86.cache-2
  0.51%        152 BSDIFF    
/usr/share/cache/fontconfig/9f60835a5eaa9f17b061039c787d35ba-x86.cache-2
  0.51%        153 BSDIFF    
/usr/share/cache/fontconfig/580bda594161061e6ef4cc8a4db09662-x86.cache-2
  0.51%        153 BSDIFF    
/usr/share/cache/fontconfig/a595ca43be94766bbfe429f07fba82d0-x86.cache-2
  0.52%        154 BSDIFF     /boot/efi/boot/bootx64.efi
  0.52%        154 BSDIFF    
/usr/share/cache/fontconfig/12b26b760a24f8b4feb03ad48a333a72-x86.cache-2
  0.52%        154 BSDIFF    
/usr/share/cache/fontconfig/aaac3452274625c4167ca2eab9cb5e70-x86.cache-2
  0.53%        159 BSDIFF     /etc/lsb-release
  0.54%        160 BSDIFF     <rootfs-inode-2-metadata>
  0.56%        167 BSDIFF     <rootfs-bg-6-0-metadata>
  0.59%        176 BSDIFF     /boot/extlinux.sys
  0.90%        269 BSDIFF     <rootfs-bg-1-2-metadata>
  1.02%        304 BSDIFF     <rootfs-bg-4-0-metadata>
  1.08%        322 BSDIFF     <rootfs-bg-5-0-metadata>
  1.15%        342 BSDIFF     <rootfs-bg-0-0-metadata>
  1.27%        380 BSDIFF     /usr/lib/libsoftokn3.chk
  1.28%        381 BSDIFF     /usr/lib/libnssdbm3.chk
  1.32%        394 BSDIFF     /usr/lib/libfreebl3.chk
  1.48%        441 BSDIFF     <rootfs-bg-2-1-metadata>
  1.82%        542 BSDIFF     <rootfs-bg-3-2-metadata>
  1.82%        544 BSDIFF     <rootfs-bg-2-0-metadata>
  2.00%        596 BSDIFF     <rootfs-bg-3-0-metadata>
  2.27%        678 BSDIFF     <rootfs-bg-1-0-metadata>
  2.62%        783 BSDIFF     <rootfs-bg-3-1-metadata>
  3.13%        933 BSDIFF     <rootfs-bg-1-1-metadata>
  6.27%       1870 -          <manifest-metadata>
  8.88%       2648 BSDIFF     <kernel-operation-0>
 53.45%      15944 REPLACE_BZ <rootfs-non-file-data>
100.00%      29832            <total>


PTAL

[petkov, 2010-12-10 00:46:55]

Good stuff. Some comments below. Where's the "other" CL that makes the image
contents align? That one is good because not only it reduces the payload size
but it also speeds up update application.

Also, could you please split out the new delta diff generator routines into a
separate file/class or something?

http://codereview.chromium.org/5684002/diff/1/SConstruct
File SConstruct (right):

http://codereview.chromium.org/5684002/diff/1/SConstruct#newcode200
SConstruct:200: ext2fs
is this library pulled into hard-host-depends somehow? does update_engine.ebuild
need an explicit dependence?

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc
File delta_diff_generator.cc (right):

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode13
delta_diff_generator.cc:13: #include <base/logging.h>
move this block of includes after the c++ headers

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode16
delta_diff_generator.cc:16: 
no need for blank line

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode423
delta_diff_generator.cc:423: // The metadata blocks are at the the start of each
block group and goes
typo: the the

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode432
delta_diff_generator.cc:432: // Due to bsdiff slowness, we're going to break
each block group down
would it be better for the payload size to not break up into chunks? are you
concerned about server side performance here or client side memory use?

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode438
delta_diff_generator.cc:438: 
remove blank line

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode447
delta_diff_generator.cc:447: for (__u32 i = curr_block; i < end_block; i++) {
there's an ExtentForRange utility in extent_ranges.h

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode452
delta_diff_generator.cc:452: metadata_name << "<rootfs-bg-" << bg << "-" <<
chunk << "-metadata>";
maybe use StringPrintf instead?

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode...
delta_diff_generator.cc:1029: // Make sure these two file systems are the same.
clarify that if they aren't the same we'll just send the metadata as a full
update.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator_unittest.cc
File delta_diff_generator_unittest.cc (right):

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator_unittest.c...
delta_diff_generator_unittest.cc:9: #include <gtest/gtest.h>
style is as below, with blank lines between groups:

c headers <>
c++ headers <>
library headers <>
project headers ""

http://codereview.chromium.org/5684002/diff/1/test_utils.cc
File test_utils.cc (right):

http://codereview.chromium.org/5684002/diff/1/test_utils.cc#newcode185
test_utils.cc:185: " seek=%Zu bs=1 count=1",
what's %Zu?

http://codereview.chromium.org/5684002/diff/1/test_utils.cc#newcode187
test_utils.cc:187: EXPECT_EQ(0, System(StringPrintf("mkfs.ext3 -b %d -F %s",
we use ext2 now, right?

http://codereview.chromium.org/5684002/diff/1/test_utils.h
File test_utils.h (right):

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode13
test_utils.h:13: #include "base/string_util.h"
since you've touched the headers -- "new" style is use <> for the library
includes, sort them together with gtest and add blank lines...

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode96
test_utils.h:96: // Creates an empty ext image.
ext2, ext3, ext4?

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode225
test_utils.h:225: EXPECT_TRUE(utils::MakeTempDirectory("/tmp/mnt.XXXXXX",
mnt_path));
lots of code for a header file. could you please move the implementation into
the .cc file?

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode236
test_utils.h:236: private:
blank line before private

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode237
test_utils.h:237: scoped_ptr<ScopedDirRemover> dir_remover_;
i guess this is not new code but... is there any rule that says what order these
objects will be destructed in? if no rule, then random order is bad.

[petkov, 2010-12-10 01:05:45]

Btw, issue 7220 is closed. Can you file a new issue to cover what you're
improving specifically and change the CL description?

[adlr, 2010-12-13 21:15:07]

Thieu, if you've split parts of this out or made fixes based on Darin's
comments, can you re-upload (if you're ready for more eyes to look at it)?

Thanks,

On Thu, Dec 9, 2010 at 5:05 PM, <petkov@chromium.org> wrote:

> Btw, issue 7220 is closed. Can you file a new issue to cover what you're
> improving specifically and change the CL description?
>
>
>
> http://codereview.chromium.org/5684002/
>

[thieule, 2010-12-14 23:11:20]

I've addressed the code review feedbacks and have also separated out the new
metadata code into its own files.

PTAL

http://codereview.chromium.org/5684002/diff/1/SConstruct
File SConstruct (right):

http://codereview.chromium.org/5684002/diff/1/SConstruct#newcode200
SConstruct:200: ext2fs
On 2010/12/10 00:46:55, petkov wrote:
> is this library pulled into hard-host-depends somehow? does
update_engine.ebuild
> need an explicit dependence?

Done.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc
File delta_diff_generator.cc (right):

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode13
delta_diff_generator.cc:13: #include <base/logging.h>
On 2010/12/10 00:46:55, petkov wrote:
> move this block of includes after the c++ headers

Done.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode16
delta_diff_generator.cc:16: 
On 2010/12/10 00:46:55, petkov wrote:
> no need for blank line

Done.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode423
delta_diff_generator.cc:423: // The metadata blocks are at the the start of each
block group and goes
On 2010/12/10 00:46:55, petkov wrote:
> typo: the the

Done.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode432
delta_diff_generator.cc:432: // Due to bsdiff slowness, we're going to break
each block group down
On 2010/12/10 00:46:55, petkov wrote:
> would it be better for the payload size to not break up into chunks? are you
> concerned about server side performance here or client side memory use?

I ran this without breaking up into chunks and bsdiff takes ~45 minutes per
block group.  It ends up taking around 4-5 hours to generate a diff.  I
investigated further and it looks like because of the large runs of similar
values (ie. zeroes and 255), bsdiff algorithm doesn't handle it well.  The
difference in payload size was negligible.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode438
delta_diff_generator.cc:438: 
On 2010/12/10 00:46:55, petkov wrote:
> remove blank line

Done.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode447
delta_diff_generator.cc:447: for (__u32 i = curr_block; i < end_block; i++) {
On 2010/12/10 00:46:55, petkov wrote:
> there's an ExtentForRange utility in extent_ranges.h

Done.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode452
delta_diff_generator.cc:452: metadata_name << "<rootfs-bg-" << bg << "-" <<
chunk << "-metadata>";
On 2010/12/10 00:46:55, petkov wrote:
> maybe use StringPrintf instead?

Done.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator.cc#newcode...
delta_diff_generator.cc:1029: // Make sure these two file systems are the same.
On 2010/12/10 00:46:55, petkov wrote:
> clarify that if they aren't the same we'll just send the metadata as a full
> update.

Done.

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator_unittest.cc
File delta_diff_generator_unittest.cc (right):

http://codereview.chromium.org/5684002/diff/1/delta_diff_generator_unittest.c...
delta_diff_generator_unittest.cc:9: #include <gtest/gtest.h>
On 2010/12/10 00:46:55, petkov wrote:
> style is as below, with blank lines between groups:
> 
> c headers <>
> c++ headers <>
> library headers <>
> project headers ""

Done.

http://codereview.chromium.org/5684002/diff/1/test_utils.cc
File test_utils.cc (right):

http://codereview.chromium.org/5684002/diff/1/test_utils.cc#newcode185
test_utils.cc:185: " seek=%Zu bs=1 count=1",
On 2010/12/10 00:46:55, petkov wrote:
> what's %Zu?

Looks like %Zu is outdated and the proper format is %zu for size_t.

http://codereview.chromium.org/5684002/diff/1/test_utils.cc#newcode187
test_utils.cc:187: EXPECT_EQ(0, System(StringPrintf("mkfs.ext3 -b %d -F %s",
On 2010/12/10 00:46:55, petkov wrote:
> we use ext2 now, right?

This is a little bit more involved than just here.  I'll do the ext3 to ext2
change in a separate CL.

http://codereview.chromium.org/5684002/diff/1/test_utils.h
File test_utils.h (right):

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode13
test_utils.h:13: #include "base/string_util.h"
On 2010/12/10 00:46:55, petkov wrote:
> since you've touched the headers -- "new" style is use <> for the library
> includes, sort them together with gtest and add blank lines...

Done.

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode96
test_utils.h:96: // Creates an empty ext image.
On 2010/12/10 00:46:55, petkov wrote:
> ext2, ext3, ext4?

This is a little bit more involved than just here.  I'll do the ext3 to ext2
change in a separate CL.

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode225
test_utils.h:225: EXPECT_TRUE(utils::MakeTempDirectory("/tmp/mnt.XXXXXX",
mnt_path));
On 2010/12/10 00:46:55, petkov wrote:
> lots of code for a header file. could you please move the implementation into
> the .cc file?

Done.

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode236
test_utils.h:236: private:
On 2010/12/10 00:46:55, petkov wrote:
> blank line before private

Done.

http://codereview.chromium.org/5684002/diff/1/test_utils.h#newcode237
test_utils.h:237: scoped_ptr<ScopedDirRemover> dir_remover_;
On 2010/12/10 00:46:55, petkov wrote:
> i guess this is not new code but... is there any rule that says what order
these
> objects will be destructed in? if no rule, then random order is bad.

There is an explicit order that these objects must be destructed.  I've added a
comment to indicate such order.

[thieule, 2010-12-15 00:38:58]

Forgot to add the new metadata files to the CL.

PTAL

[adlr, 2010-12-15 03:11:22]

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h
File delta_diff_generator.h (right):

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h#newc...
delta_diff_generator.h:227: bool BsdiffFiles(const std::string& old_file,
can these be static methods of DeltaDiffGenerator (following convention above)

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator_unitte...
File delta_diff_generator_unittest.cc (right):

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator_unitte...
delta_diff_generator_unittest.cc:18: #include "base/logging.h"
move the base headers up to the library section (w/ gtest) and use <> rather
than ""

http://codereview.chromium.org/5684002/diff/15001/metadata.cc
File metadata.cc (right):

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode28
metadata.cc:28: const size_t kBlockSize = 4096;
unindent

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode46
metadata.cc:46: // Read in the data blocks
would "metadata blocks" be more appropriate?

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode47
metadata.cc:47: const size_t max_read_blocks = 256;
kMaxReadBlocks

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode55
metadata.cc:55: static_cast<vector<char>::size_type>(
why vector<char>? you could just use size_t or vector<Block> to be consistent

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode102
metadata.cc:102: CHECK_GT(bsdiff_delta->size(), 0);
do you really want to crash if this fails? would TEST_AND_RETURN_FALSE() be
better?

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode118
metadata.cc:118: // Read in the metadata blocks from the old and new image.
add { (more below)

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode161
metadata.cc:161: 
add } here

this deallocates the extra copy/copies of data

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode302
metadata.cc:302: errcode_t error = ext2fs_get_next_inode(iscan, &ino,
&old_inode);
is there a TEST_AND_RETURN_FALSE* thing you can use for this?

http://codereview.chromium.org/5684002/diff/15001/metadata_unittest.cc
File metadata_unittest.cc (right):

http://codereview.chromium.org/5684002/diff/15001/metadata_unittest.cc#newcode73
metadata_unittest.cc:73: const size_t image_size = (256 * 1024 * 1024); //
Enough for 2 block groups
extra space before //

[petkov, 2010-12-15 18:40:32]

Mostly style nits.

Is this change somewhat dependent on the build_image change? I.e., what happens
if you things are not aligned -- is the payload size going to be bigger/smaller
than without this change?

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h
File delta_diff_generator.h (right):

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h#newc...
delta_diff_generator.h:231: // The blocks vector contains a reader and writer
for each block on the
s/blocks/|blocks|/

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h#newc...
delta_diff_generator.h:234: // For each block in 'operation' that is read or
written, find that block
s/'operation'/|operation|/

same for blocks, graph below.

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h#newc...
delta_diff_generator.h:240: std::vector<DeltaDiffGenerator::Block>* blocks,
it might be good, per style, to move |blocks| to be the last parameter is it's
in/out while all the other parameters are strictly inputs.

http://codereview.chromium.org/5684002/diff/15001/metadata.cc
File metadata.cc (right):

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode34
metadata.cc:34: bool ReadExtentsData(const ext2_filsys fs,
this and all other internal only routines should be part of the anonymous
namespace

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode292
metadata.cc:292: 
remove blank line

http://codereview.chromium.org/5684002/diff/15001/metadata.h
File metadata.h (right):

http://codereview.chromium.org/5684002/diff/15001/metadata.h#newcode21
metadata.h:21: bool DeltaReadMetadata(Graph* graph,
This should probably be a static member of a class. See full_update_generator.h,
for example.

http://codereview.chromium.org/5684002/diff/15001/test_utils.h
File test_utils.h (right):

http://codereview.chromium.org/5684002/diff/15001/test_utils.h#newcode13
test_utils.h:13: #include <base/string_util.h>
do you need this header file here? can it be moved to the .cc file instead?

http://codereview.chromium.org/5684002/diff/15001/utils.h
File utils.h (right):

http://codereview.chromium.org/5684002/diff/15001/utils.h#newcode280
utils.h:280: private:
empty line before private (yeah, i know the rest of the classes don't have it
but they should)

http://codereview.chromium.org/5684002/diff/15001/utils.h#newcode282
utils.h:282: };
Add DISALLOW_COPY_AND_ASSIGN

[thieule, 2010-12-15 19:57:01]

Addressed code review feedbacks.

PTAL

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h
File delta_diff_generator.h (right):

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h#newc...
delta_diff_generator.h:227: bool BsdiffFiles(const std::string& old_file,
On 2010/12/15 03:11:23, adlr wrote:
> can these be static methods of DeltaDiffGenerator (following convention above)

Done.

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h#newc...
delta_diff_generator.h:231: // The blocks vector contains a reader and writer
for each block on the
On 2010/12/15 18:40:32, petkov wrote:
> s/blocks/|blocks|/

Done.

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h#newc...
delta_diff_generator.h:234: // For each block in 'operation' that is read or
written, find that block
On 2010/12/15 18:40:32, petkov wrote:
> s/'operation'/|operation|/
> 
> same for blocks, graph below.

Done.

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator.h#newc...
delta_diff_generator.h:240: std::vector<DeltaDiffGenerator::Block>* blocks,
On 2010/12/15 18:40:32, petkov wrote:
> it might be good, per style, to move |blocks| to be the last parameter is it's
> in/out while all the other parameters are strictly inputs.

Done.

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator_unitte...
File delta_diff_generator_unittest.cc (right):

http://codereview.chromium.org/5684002/diff/15001/delta_diff_generator_unitte...
delta_diff_generator_unittest.cc:18: #include "base/logging.h"
On 2010/12/15 03:11:23, adlr wrote:
> move the base headers up to the library section (w/ gtest) and use <> rather
> than ""

Done.

http://codereview.chromium.org/5684002/diff/15001/metadata.cc
File metadata.cc (right):

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode28
metadata.cc:28: const size_t kBlockSize = 4096;
On 2010/12/15 03:11:23, adlr wrote:
> unindent

Done.

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode34
metadata.cc:34: bool ReadExtentsData(const ext2_filsys fs,
On 2010/12/15 18:40:32, petkov wrote:
> this and all other internal only routines should be part of the anonymous
> namespace

Done.

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode46
metadata.cc:46: // Read in the data blocks
ReadExtentsData is generic and just reads data from the specified |extents|.  It
could be metadata, free space, etc.

On 2010/12/15 03:11:23, adlr wrote:
> would "metadata blocks" be more appropriate?

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode47
metadata.cc:47: const size_t max_read_blocks = 256;
On 2010/12/15 03:11:23, adlr wrote:
> kMaxReadBlocks

Done.

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode55
metadata.cc:55: static_cast<vector<char>::size_type>(
Had a synapse misfire here.

On 2010/12/15 03:11:23, adlr wrote:
> why vector<char>? you could just use size_t or vector<Block> to be consistent

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode102
metadata.cc:102: CHECK_GT(bsdiff_delta->size(), 0);
I've removed this, it's a redundant check.  The better place to check this is
below in AddMetadataExtents() where there is more context as to why the
bsdiff_delta->size() must be greater than zero (since it's only called when we
know the two metadata are different).

On 2010/12/15 03:11:23, adlr wrote:
> do you really want to crash if this fails? would TEST_AND_RETURN_FALSE() be
> better?

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode118
metadata.cc:118: // Read in the metadata blocks from the old and new image.
On 2010/12/15 03:11:23, adlr wrote:
> add { (more below)

Done.

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode161
metadata.cc:161: 
On 2010/12/15 03:11:23, adlr wrote:
> add } here
> 
> this deallocates the extra copy/copies of data

Done.

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode292
metadata.cc:292: 
On 2010/12/15 18:40:32, petkov wrote:
> remove blank line

Done.

http://codereview.chromium.org/5684002/diff/15001/metadata.cc#newcode302
metadata.cc:302: errcode_t error = ext2fs_get_next_inode(iscan, &ino,
&old_inode);
If we return false here, it will cause the delta_generator to exit with an
error.  By exiting here with a break which leads to a successful return code,
the delta_generator will treat the missing blocks as non-file data blocks and
compress them as an entire blob in ReadUnwrittenBlocks().  Since we have this
failsafe fallback, I don't think we need to error out here.  I've added comments
to reflect this.

On 2010/12/15 03:11:23, adlr wrote:
> is there a TEST_AND_RETURN_FALSE* thing you can use for this?

http://codereview.chromium.org/5684002/diff/15001/metadata.h
File metadata.h (right):

http://codereview.chromium.org/5684002/diff/15001/metadata.h#newcode21
metadata.h:21: bool DeltaReadMetadata(Graph* graph,
On 2010/12/15 18:40:32, petkov wrote:
> This should probably be a static member of a class. See
full_update_generator.h,
> for example.

Done.

http://codereview.chromium.org/5684002/diff/15001/metadata_unittest.cc
File metadata_unittest.cc (right):

http://codereview.chromium.org/5684002/diff/15001/metadata_unittest.cc#newcode73
metadata_unittest.cc:73: const size_t image_size = (256 * 1024 * 1024); //
Enough for 2 block groups
On 2010/12/15 03:11:23, adlr wrote:
> extra space before //

Done.

http://codereview.chromium.org/5684002/diff/15001/test_utils.h
File test_utils.h (right):

http://codereview.chromium.org/5684002/diff/15001/test_utils.h#newcode13
test_utils.h:13: #include <base/string_util.h>
On 2010/12/15 18:40:32, petkov wrote:
> do you need this header file here? can it be moved to the .cc file instead?

Done.

http://codereview.chromium.org/5684002/diff/15001/utils.h
File utils.h (right):

http://codereview.chromium.org/5684002/diff/15001/utils.h#newcode280
utils.h:280: private:
On 2010/12/15 18:40:32, petkov wrote:
> empty line before private (yeah, i know the rest of the classes don't have it
> but they should)

Done.

http://codereview.chromium.org/5684002/diff/15001/utils.h#newcode282
utils.h:282: };
On 2010/12/15 18:40:32, petkov wrote:
> Add DISALLOW_COPY_AND_ASSIGN

Done.

[petkov, 2010-12-15 23:30:16]

Looks good, one question below.

Also, how does this change play in the presence of base image in build_image?
Would splitting the meta data in multiple chunks hurt the delta payload size if
the target and source images are not aligned in any way?

http://codereview.chromium.org/5684002/diff/28001/test_utils.h
File test_utils.h (right):

http://codereview.chromium.org/5684002/diff/28001/test_utils.h#newcode228
test_utils.h:228: // These objects must be destructed in the following order:
Does C++ standard ensure the destruction order?

[thieule, 2010-12-15 23:43:24]

So I ran a test using two unaligned images.  The first test uses the original
(non-metadata aware) delta_generator and the second test uses the metadata aware
version.  The difference is negligible.

Orig:
100.00%     719370            <total>

Metadata:
100.00%     719115            <total>

On 2010/12/15 23:30:16, petkov wrote:
> Looks good, one question below.
> 
> Also, how does this change play in the presence of base image in build_image?
> Would splitting the meta data in multiple chunks hurt the delta payload size
if
> the target and source images are not aligned in any way?
> 
> http://codereview.chromium.org/5684002/diff/28001/test_utils.h
> File test_utils.h (right):
> 
> http://codereview.chromium.org/5684002/diff/28001/test_utils.h#newcode228
> test_utils.h:228: // These objects must be destructed in the following order:
> Does C++ standard ensure the destruction order?

[thieule, 2010-12-15 23:43:39]

http://codereview.chromium.org/5684002/diff/28001/test_utils.h
File test_utils.h (right):

http://codereview.chromium.org/5684002/diff/28001/test_utils.h#newcode228
test_utils.h:228: // These objects must be destructed in the following order:
Yes, nonstatic members are destroyed in reverse order.

On 2010/12/15 23:30:16, petkov wrote:
> Does C++ standard ensure the destruction order?

[petkov, 2010-12-15 23:47:05]

LGTM

[adlr, 2010-12-15 23:51:49]

LGTM from me as well!

Thanks, Thieu!