AU: Gut code for old updater. New protobuf for v2 updater.

AU: Gut code for old updater. New protobuf for v2 updater.

[adlr, 2010-01-15 00:10:01]

Dan,

- The doc for this isn't on the site (just) yet, but it will be shortly (in a
day or two, it sounds like).

- If you don't want these reviews, let me know. You've been great w/ the
reviewing, but if you feel overloaded, let me know and I'll look for someone
else to review.

- There are a couple lines of source commented out. I know that's frowned up on
in general, but it's in part of the code that's mostly going to stay the same,
but needs some lines missing so it will compile. Hopefully that's okay, tho if
pressed I can remove the lines.

Thanks,
-andrew

[Daniel Erat, 2010-01-15 23:13:51]

http://codereview.chromium.org/545072/diff/1001/2003
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/545072/diff/1001/2003#newcode12
src/platform/update_engine/delta_diff_generator.h:12: class DeltaDiffGenerator {
High-level question: any reason that you're not just deleting this and the .cc
file?  They don't seem to really contain anything now.  It's fine if you're
planning to put stuff back in in a later CL; just curious.

http://codereview.chromium.org/545072/diff/1001/2005
File src/platform/update_engine/delta_diff_parser.cc (right):

http://codereview.chromium.org/545072/diff/1001/2005#newcode20
src/platform/update_engine/delta_diff_parser.cc:20: #include
"update_engine/utils.h"
re earlier comment, if you're going to fill these back in later, you should
remove the extraneous includes now so it's less likely that they'll accidentally
stay in

http://codereview.chromium.org/545072/diff/1001/2014
File src/platform/update_engine/install_plan.h (right):

http://codereview.chromium.org/545072/diff/1001/2014#newcode20
src/platform/update_engine/install_plan.h:20: const std::string& i_path)
nit: 'i_path' is pretty ambiguous.  naming the param 'install_path' and then
just doing "install_path(install_path)" in the initialization list should work

http://codereview.chromium.org/545072/diff/1001/2015
File src/platform/update_engine/main.cc (right):

http://codereview.chromium.org/545072/diff/1001/2015#newcode5
src/platform/update_engine/main.cc:5: // TODO(adlr): get rid of commented out
lines or comment them back in.
I don't have any great suggestions here, but if you can think of any way to mark
these lines (like a "// re-add" trailing comment), it might make them easier to
find later.

http://codereview.chromium.org/545072/diff/1001/2019
File src/platform/update_engine/update_metadata.proto (right):

http://codereview.chromium.org/545072/diff/1001/2019#newcode10
src/platform/update_engine/update_metadata.proto:10: //   uint32 version_number
= 1;
is this the version number of this particular update, or does it refer to
different versions of the update file format?  if there's some way to rename
this to make it obvious, that'd be good.

http://codereview.chromium.org/545072/diff/1001/2019#newcode11
src/platform/update_engine/update_metadata.proto:11: //   uint64 manifest_size; 
// Sise of protobuf DeltaArchiveManifest
s/Sise/Size/

http://codereview.chromium.org/545072/diff/1001/2019#newcode47
src/platform/update_engine/update_metadata.proto:47: optional uint64 start = 1; 
// blocks
rename these to 'start_block' and 'num_blocks'

http://codereview.chromium.org/545072/diff/1001/2019#newcode48
src/platform/update_engine/update_metadata.proto:48: optional uint64 length = 2;
 // blocks
switch these to required?  if not, what do missing values mean?

http://codereview.chromium.org/545072/diff/1001/2019#newcode57
src/platform/update_engine/update_metadata.proto:57: BSDIFF = 2;  // The data is
a bsdiff binary diff
s/2/3/

http://codereview.chromium.org/545072/diff/1001/2019#newcode69
src/platform/update_engine/update_metadata.proto:69: optional uint64 src_length
= 5;
is this always equal to the sum of src_extents.length * block_size, or does it
have some other meaning?

http://codereview.chromium.org/545072/diff/1001/2019#newcode79
src/platform/update_engine/update_metadata.proto:79: optional uint32 block_size
= 5;  // (At time of writing) usually 4096
does it make sense to set a default for this?

[adlr, 2010-01-21 22:17:22]

ready for another look. thanks

http://codereview.chromium.org/545072/diff/1001/2003
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/545072/diff/1001/2003#newcode12
src/platform/update_engine/delta_diff_generator.h:12: class DeltaDiffGenerator {
On 2010/01/15 23:13:51, Daniel Erat wrote:
> High-level question: any reason that you're not just deleting this and the .cc
> file?  They don't seem to really contain anything now.  It's fine if you're
> planning to put stuff back in in a later CL; just curious.

Yeah, I will have a diff generator class (the other CL is in my queue). For
things that I really don't need anymore, I deleted them.

http://codereview.chromium.org/545072/diff/1001/2005
File src/platform/update_engine/delta_diff_parser.cc (right):

http://codereview.chromium.org/545072/diff/1001/2005#newcode20
src/platform/update_engine/delta_diff_parser.cc:20: #include
"update_engine/utils.h"
On 2010/01/15 23:13:51, Daniel Erat wrote:
> re earlier comment, if you're going to fill these back in later, you should
> remove the extraneous includes now so it's less likely that they'll
accidentally
> stay in

Done.

http://codereview.chromium.org/545072/diff/1001/2014
File src/platform/update_engine/install_plan.h (right):

http://codereview.chromium.org/545072/diff/1001/2014#newcode20
src/platform/update_engine/install_plan.h:20: const std::string& i_path)
On 2010/01/15 23:13:51, Daniel Erat wrote:
> nit: 'i_path' is pretty ambiguous.  naming the param 'install_path' and then
> just doing "install_path(install_path)" in the initialization list should work

Done.

http://codereview.chromium.org/545072/diff/1001/2015
File src/platform/update_engine/main.cc (right):

http://codereview.chromium.org/545072/diff/1001/2015#newcode5
src/platform/update_engine/main.cc:5: // TODO(adlr): get rid of commented out
lines or comment them back in.
On 2010/01/15 23:13:51, Daniel Erat wrote:
> I don't have any great suggestions here, but if you can think of any way to
mark
> these lines (like a "// re-add" trailing comment), it might make them easier
to
> find later.

Done.

http://codereview.chromium.org/545072/diff/1001/2019
File src/platform/update_engine/update_metadata.proto (right):

http://codereview.chromium.org/545072/diff/1001/2019#newcode10
src/platform/update_engine/update_metadata.proto:10: //   uint32 version_number
= 1;
On 2010/01/15 23:13:51, Daniel Erat wrote:
> is this the version number of this particular update, or does it refer to
> different versions of the update file format?  if there's some way to rename
> this to make it obvious, that'd be good.

Done.

http://codereview.chromium.org/545072/diff/1001/2019#newcode11
src/platform/update_engine/update_metadata.proto:11: //   uint64 manifest_size; 
// Sise of protobuf DeltaArchiveManifest
On 2010/01/15 23:13:51, Daniel Erat wrote:
> s/Sise/Size/

Done.

http://codereview.chromium.org/545072/diff/1001/2019#newcode47
src/platform/update_engine/update_metadata.proto:47: optional uint64 start = 1; 
// blocks
On 2010/01/15 23:13:51, Daniel Erat wrote:
> rename these to 'start_block' and 'num_blocks'

Done.

http://codereview.chromium.org/545072/diff/1001/2019#newcode48
src/platform/update_engine/update_metadata.proto:48: optional uint64 length = 2;
 // blocks
On 2010/01/15 23:13:51, Daniel Erat wrote:
> switch these to required?  if not, what do missing values mean?

afaik, "required" means that protobuf code might CHECK() if they are missing.
optional means I can handle their being missing as I see fit (ie, not crashing).

http://codereview.chromium.org/545072/diff/1001/2019#newcode57
src/platform/update_engine/update_metadata.proto:57: BSDIFF = 2;  // The data is
a bsdiff binary diff
On 2010/01/15 23:13:51, Daniel Erat wrote:
> s/2/3/

Done.

http://codereview.chromium.org/545072/diff/1001/2019#newcode69
src/platform/update_engine/update_metadata.proto:69: optional uint64 src_length
= 5;
On 2010/01/15 23:13:51, Daniel Erat wrote:
> is this always equal to the sum of src_extents.length * block_size, or does it
> have some other meaning?

it may not be block-size aligned. I actually may get rid of this, since I don't
think we need it for src, but for dst_lenght, we do need it, so that we know how
long the length is non-block-aligned.

http://codereview.chromium.org/545072/diff/1001/2019#newcode79
src/platform/update_engine/update_metadata.proto:79: optional uint32 block_size
= 5;  // (At time of writing) usually 4096
On 2010/01/15 23:13:51, Daniel Erat wrote:
> does it make sense to set a default for this?

Done.

[Daniel Erat, 2010-01-22 02:17:38]

http://codereview.chromium.org/545072/diff/5001/40
File src/platform/update_engine/SConstruct (right):

http://codereview.chromium.org/545072/diff/5001/40#newcode5
src/platform/update_engine/SConstruct:5: import os
do you need this?  doesn't look like you're adding anything that would use it

http://codereview.chromium.org/545072/diff/5001/58
File src/platform/update_engine/update_metadata.proto (right):

http://codereview.chromium.org/545072/diff/5001/58#newcode10
src/platform/update_engine/update_metadata.proto:10: //   uint32
file_format_version_number = 1;
i think it makes sense to drop the "_number" from this to make it a bit shorter;
it doesn't add much

http://codereview.chromium.org/545072/diff/5001/58#newcode47
src/platform/update_engine/update_metadata.proto:47: optional uint64 start_block
= 1;  // blocks
the "blocks" comments are unneeded now

http://codereview.chromium.org/545072/diff/5001/58#newcode69
src/platform/update_engine/update_metadata.proto:69: repeated Extent dst_extents
= 6;
move this up one line since the comment above 'src_extents' refers to it. 
renumber from 6 to 5?

http://codereview.chromium.org/545072/diff/5001/58#newcode70
src/platform/update_engine/update_metadata.proto:70: // byte length of dst, not
necessarily block aligned
i still don't understand from the comment and the field name what this actually
refers to.

http://codereview.chromium.org/545072/diff/5001/58#newcode79
src/platform/update_engine/update_metadata.proto:79: optional uint32 block_size
= 5 [default = 4096];
renumber from 5 to 4?

[adlr, 2010-01-22 20:32:55]

http://codereview.chromium.org/545072/diff/5001/40
File src/platform/update_engine/SConstruct (right):

http://codereview.chromium.org/545072/diff/5001/40#newcode5
src/platform/update_engine/SConstruct:5: import os
On 2010/01/22 02:17:38, Daniel Erat wrote:
> do you need this?  doesn't look like you're adding anything that would use it

I need it for this part:

# Fix issue with scons not passing pkg-config vars through the environment.
for key in Split('PKG_CONFIG_LIBDIR PKG_CONFIG_PATH'):
  if os.environ.has_key(key):
    env['ENV'][key] = os.environ[key]

http://codereview.chromium.org/545072/diff/5001/58
File src/platform/update_engine/update_metadata.proto (right):

http://codereview.chromium.org/545072/diff/5001/58#newcode10
src/platform/update_engine/update_metadata.proto:10: //   uint32
file_format_version_number = 1;
On 2010/01/22 02:17:38, Daniel Erat wrote:
> i think it makes sense to drop the "_number" from this to make it a bit
shorter;
> it doesn't add much

Done.

http://codereview.chromium.org/545072/diff/5001/58#newcode47
src/platform/update_engine/update_metadata.proto:47: optional uint64 start_block
= 1;  // blocks
On 2010/01/22 02:17:38, Daniel Erat wrote:
> the "blocks" comments are unneeded now

Done.

http://codereview.chromium.org/545072/diff/5001/58#newcode69
src/platform/update_engine/update_metadata.proto:69: repeated Extent dst_extents
= 6;
On 2010/01/22 02:17:38, Daniel Erat wrote:
> move this up one line since the comment above 'src_extents' refers to it. 
> renumber from 6 to 5?

I actually realized that I do need src_length, so I put it back in, thus
obviating the need for renumbering.

http://codereview.chromium.org/545072/diff/5001/58#newcode70
src/platform/update_engine/update_metadata.proto:70: // byte length of dst, not
necessarily block aligned
On 2010/01/22 02:17:38, Daniel Erat wrote:
> i still don't understand from the comment and the field name what this
actually
> refers to.

Each op can have (optionally) some source data and some dest data. That data is
an ordered list of blocks on the install partition. However, the source and dest
length might now be block aligned. For example, say we want to write a file of
length 2 with data "hi". That data will live in a block, so the dst_extents will
contain one extent with num_blocks = 1 and start_block = the block where "hi"
lives. dst_length will be the length of the data in bytes (here, 2). Does that
make sense?

It comes into play more with bsdiff. We want to read the source file off disk
which lives in certain blocks, but the source file's length is not necessarily
block-aligned. Same for the dest.

http://codereview.chromium.org/545072/diff/5001/58#newcode79
src/platform/update_engine/update_metadata.proto:79: optional uint32 block_size
= 5 [default = 4096];
On 2010/01/22 02:17:38, Daniel Erat wrote:
> renumber from 5 to 4?

see earlier comment.

[adlr, 2010-01-22 22:43:03]

added comments based on our gchat. please have another look. thanks,

[Daniel Erat, 2010-01-22 22:51:33]

Looks great.  Thanks!

http://codereview.chromium.org/545072/diff/10001/8039
File src/platform/update_engine/update_metadata.proto (right):

http://codereview.chromium.org/545072/diff/10001/8039#newcode56
src/platform/update_engine/update_metadata.proto:56: // A seminal value
(kuint64max) as the start block denotes a sparse-hole
s/seminal/sentinel/