AU: Class to perform delta updates.

AU: Class to perform delta updates.

A class to perform delta updates and the associated unittests. Also,
change the delta diff generator executable to be able to apply a
delta, which is handy for debugging.

TEST=Attached unit test, hand-tested on real build images
BUG=552

[adlr, 2010-04-20 20:59:07]

Again, it's not as bad as it looks. One major class + unittest
(delta_performer*), and a bunch of minor edits to other files.

[Daniel Erat, 2010-04-20 23:52:38]

http://codereview.chromium.org/1718001/diff/1/9
File src/platform/update_engine/delta_performer.cc (right):

http://codereview.chromium.org/1718001/diff/1/9#newcode12
src/platform/update_engine/delta_performer.cc:12: #include "base/scoped_ptr.h"
the include order here seems kinda weird -- move the "base/..." includes into
the same block as the "update_engine/..." ones?

http://codereview.chromium.org/1718001/diff/1/9#newcode32
src/platform/update_engine/delta_performer.cc:32: void
RemoveBufferHeadBytes(std::vector<char>* buffer, size_t count) {
add a DCHECK_GE(buffer->size(), count)

you don't need std:: before vector here

http://codereview.chromium.org/1718001/diff/1/9#newcode33
src/platform/update_engine/delta_performer.cc:33: memmove(&(*buffer)[0],
&(*buffer)[count], buffer->size() - count);
can you just use buffer->erase() instead?

http://codereview.chromium.org/1718001/diff/1/9#newcode54
src/platform/update_engine/delta_performer.cc:54: fd_ = -2;
why -2 (vs. -1)?  if it's to prevent Open() from being called again, it feels a
bit too tricky (at least without documenting this above fd_ in the header)

http://codereview.chromium.org/1718001/diff/1/9#newcode59
src/platform/update_engine/delta_performer.cc:59: namespace {
move these up to the top of the file, where another function is already being
declared in an anon namespace?

http://codereview.chromium.org/1718001/diff/1/9#newcode90
src/platform/update_engine/delta_performer.cc:90: case
DeltaArchiveManifest_InstallOperation_Type_REPLACE:
i think that there are methods generated to convert enum values to strings. 
maybe DeltaArchiveManifest_InstallOperation_Type_Name() or
DeltaArchiveManifest::InstallOperation::Type_Name() or something?

http://codereview.chromium.org/1718001/diff/1/9#newcode132
src/platform/update_engine/delta_performer.cc:132: uint64 protobuf_length;
use _t here and elsewhere

http://codereview.chromium.org/1718001/diff/1/9#newcode135
src/platform/update_engine/delta_performer.cc:135: memcpy(&protobuf_length,
#include <cstring> for this

http://codereview.chromium.org/1718001/diff/1/9#newcode138
src/platform/update_engine/delta_performer.cc:138: protobuf_length =
be64toh(protobuf_length);  // switch big endian to host
#include <endian.h> for this

http://codereview.chromium.org/1718001/diff/1/9#newcode146
src/platform/update_engine/delta_performer.cc:146: if
(!manifest_.ParseFromArray(&buffer_[offset], protobuf_length)) {
does ParseFromArray() already log errors?  if not, you should do it yourself
before returning

http://codereview.chromium.org/1718001/diff/1/9#newcode152
src/platform/update_engine/delta_performer.cc:152:
buffer_.erase(buffer_.begin(), buffer_.begin() + strlen(kDeltaMagic) +
can you use RemoveBufferHeadBytes() here instead?

http://codereview.chromium.org/1718001/diff/1/9#newcode204
src/platform/update_engine/delta_performer.cc:204: const
DeltaArchiveManifest_InstallOperation& operation) {
CHECK(operation.type() == DeltaArchiveManifest_InstallOperation_Type_REPLACE ||
operation.type() == DeltaArchiveManifest_InstallOperation_Type_REPLACE_BZ)

(you check this later, but it'd probably be good to explicitly check it at the
top of the method and then just use NOTREACHED() in the else section below)

http://codereview.chromium.org/1718001/diff/1/9#newcode225
src/platform/update_engine/delta_performer.cc:225: TEST_AND_RETURN_FALSE(false);
why not just "return false"?  the only additional thing that
TEST_AND_RETURN_FALSE() will do is print a useless "false failed." error message

http://codereview.chromium.org/1718001/diff/1/9#newcode246
src/platform/update_engine/delta_performer.cc:246: // Calculate buffer size.
Note, this function doesn't do a slideing
s/slideing/sliding/

http://codereview.chromium.org/1718001/diff/1/9#newcode252
src/platform/update_engine/delta_performer.cc:252: for (int i = 0; i <
operation.src_extents_size(); i++) {
be consistent about whether single-line if/for/while statements have curly
braces or not (ExtentsToBsdiffPositionsString() later in the file doesn't use
braces)

http://codereview.chromium.org/1718001/diff/1/9#newcode267
src/platform/update_engine/delta_performer.cc:267: ssize_t bytes_read_iter = 0;
choose a better name?  "iter" makes me think it's an stl iterator

http://codereview.chromium.org/1718001/diff/1/9#newcode301
src/platform/update_engine/delta_performer.cc:301: for (int i = 0; i <
extents.size() && length < full_length; i++) {
'i' should hit extents.size() at the same time that length hits full_length,
right?  if so, why not only use the 'i < extents.size()' condition here, and
then DCHECK_EQ(length, full_length) after the loop is done?  (it looks like
you're already sorta checking the same thing in the DCHECK within the loop)

http://codereview.chromium.org/1718001/diff/1/9#newcode337
src/platform/update_engine/delta_performer.cc:337:
TEST_AND_RETURN_FALSE(utils::MakeTempFile("/tmp/au_patch.XXXXXX",
doesn't need to be part of this CL, but it'd be better if you made the temp
directory be a flag or constant shared across all this code instead of
hardcoding it in many places

http://codereview.chromium.org/1718001/diff/1/10
File src/platform/update_engine/delta_performer.h (right):

http://codereview.chromium.org/1718001/diff/1/10#newcode38
src/platform/update_engine/delta_performer.h:38: // The length value of the last
extent in the string may be stort, since
s/stort/short/ ?

http://codereview.chromium.org/1718001/diff/1/10#newcode40
src/platform/update_engine/delta_performer.h:40: // Also, an extent starting at
kSpareseHole, appears as -1 in the string.
s/kSpareseHole,/kSparseHole/

http://codereview.chromium.org/1718001/diff/1/10#newcode41
src/platform/update_engine/delta_performer.h:41: // For example, if the Extents
are {1, 1}, {4, 2}, {kSpareseHole, 1},
s/kSpareseHole/kSparseHole/

http://codereview.chromium.org/1718001/diff/1/10#newcode46
src/platform/update_engine/delta_performer.h:46: uint64_t block_size,
include inttypes.h in this header instead of in the .cc file

http://codereview.chromium.org/1718001/diff/1/10#newcode47
src/platform/update_engine/delta_performer.h:47: uint64_t full_length);
nit: add a newline before 'private'

http://codereview.chromium.org/1718001/diff/1/10#newcode52
src/platform/update_engine/delta_performer.h:52: const
chromeos_update_engine::DeltaArchiveManifest_InstallOperation&
you shouldn't need the "chromeos_update_engine::"; you're already in this
namespace

http://codereview.chromium.org/1718001/diff/1/10#newcode62
src/platform/update_engine/delta_performer.h:62: bool PerformCopyOperation(
why "Copy" instead of "Move" (to match the enum's name in the proto)?

http://codereview.chromium.org/1718001/diff/1/10#newcode78
src/platform/update_engine/delta_performer.h:78: // Data buffered from the
binary blobs section.
this comment isn't exactly correct, is it?  buffer_ contains the beginning of
the update (that is, the protobuf) until you see all of the protobuf; then you
erase the protobuf from the beginning and use it to store the blobs

http://codereview.chromium.org/1718001/diff/1/10#newcode81
src/platform/update_engine/delta_performer.h:81: uint64 buffer_offset_;
use uint64_t

http://codereview.chromium.org/1718001/diff/1/10#newcode84
src/platform/update_engine/delta_performer.h:84: uint32 block_size_;
use uint32_t

http://codereview.chromium.org/1718001/diff/1/10#newcode85
src/platform/update_engine/delta_performer.h:85: };
add DISALLOW_COPY_AND_ASSIGN(DeltaPerformer) (you should add this to other
classes too -- the ones in file_writer.h are missing it, for instance)

http://codereview.chromium.org/1718001/diff/1/10#newcode88
src/platform/update_engine/delta_performer.h:88: const int kDeltaVersionLength =
8;
what are these doing down here?

http://codereview.chromium.org/1718001/diff/1/11
File src/platform/update_engine/delta_performer_unittest.cc (right):

http://codereview.chromium.org/1718001/diff/1/11#newcode55
src/platform/update_engine/delta_performer_unittest.cc:55: expected_output +=
StringPrintf("%" PRIu64 ":", start * block_size);
i don't think that this test accomplishes much, since it contains essentially
the same code as what it's testing.  it'd probably be better to hand-write the
expected string by looking at the test data above, and then just compare against
that

http://codereview.chromium.org/1718001/diff/1/11#newcode110
src/platform/update_engine/delta_performer_unittest.cc:110: bool
WriteSpareFile(const string& path, off_t size) {
s/WriteSpareFile/WriteSparseFile/

http://codereview.chromium.org/1718001/diff/1/20
File src/platform/update_engine/utils.cc (right):

http://codereview.chromium.org/1718001/diff/1/20#newcode67
src/platform/update_engine/utils.cc:67: offset + bytes_read);
fix indenting

http://codereview.chromium.org/1718001/diff/1/21
File src/platform/update_engine/utils.h (right):

http://codereview.chromium.org/1718001/diff/1/21#newcode188
src/platform/update_engine/utils.h:188: LOG(ERROR) << "Unable to remove dir " <<
path_ << ": " << err_message;
why not use PLOG(ERROR) instead so you don't need to call strerror()?

[adlr, 2010-04-23 04:48:11]

PTAL.

Also, thanks for doing these reviews.

http://codereview.chromium.org/1718001/diff/1/9
File src/platform/update_engine/delta_performer.cc (right):

http://codereview.chromium.org/1718001/diff/1/9#newcode12
src/platform/update_engine/delta_performer.cc:12: #include "base/scoped_ptr.h"
On 2010/04/20 23:52:38, Daniel Erat wrote:
> the include order here seems kinda weird -- move the "base/..." includes into
> the same block as the "update_engine/..." ones?

I thought base was a library. I certainly link a library called base and I
thought this was the same code...

The order I have is:
- corresponding .h
- C standard includes
- C++ standard includes
- library includes
- local includes

http://codereview.chromium.org/1718001/diff/1/9#newcode32
src/platform/update_engine/delta_performer.cc:32: void
RemoveBufferHeadBytes(std::vector<char>* buffer, size_t count) {
On 2010/04/20 23:52:38, Daniel Erat wrote:
> add a DCHECK_GE(buffer->size(), count)
> 
> you don't need std:: before vector here

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode33
src/platform/update_engine/delta_performer.cc:33: memmove(&(*buffer)[0],
&(*buffer)[count], buffer->size() - count);
On 2010/04/20 23:52:38, Daniel Erat wrote:
> can you just use buffer->erase() instead?

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode54
src/platform/update_engine/delta_performer.cc:54: fd_ = -2;
On 2010/04/20 23:52:38, Daniel Erat wrote:
> why -2 (vs. -1)?  if it's to prevent Open() from being called again, it feels
a
> bit too tricky (at least without documenting this above fd_ in the header)

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode59
src/platform/update_engine/delta_performer.cc:59: namespace {
On 2010/04/20 23:52:38, Daniel Erat wrote:
> move these up to the top of the file, where another function is already being
> declared in an anon namespace?

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode90
src/platform/update_engine/delta_performer.cc:90: case
DeltaArchiveManifest_InstallOperation_Type_REPLACE:
On 2010/04/20 23:52:38, Daniel Erat wrote:
> i think that there are methods generated to convert enum values to strings. 
> maybe DeltaArchiveManifest_InstallOperation_Type_Name() or
> DeltaArchiveManifest::InstallOperation::Type_Name() or something?

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode132
src/platform/update_engine/delta_performer.cc:132: uint64 protobuf_length;
On 2010/04/20 23:52:38, Daniel Erat wrote:
> use _t here and elsewhere

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode135
src/platform/update_engine/delta_performer.cc:135: memcpy(&protobuf_length,
On 2010/04/20 23:52:38, Daniel Erat wrote:
> #include <cstring> for this

how about just <string.h>, as specified by man memcpy? If so, done.

http://codereview.chromium.org/1718001/diff/1/9#newcode138
src/platform/update_engine/delta_performer.cc:138: protobuf_length =
be64toh(protobuf_length);  // switch big endian to host
On 2010/04/20 23:52:38, Daniel Erat wrote:
> #include <endian.h> for this

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode146
src/platform/update_engine/delta_performer.cc:146: if
(!manifest_.ParseFromArray(&buffer_[offset], protobuf_length)) {
On 2010/04/20 23:52:38, Daniel Erat wrote:
> does ParseFromArray() already log errors?  if not, you should do it yourself
> before returning

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode152
src/platform/update_engine/delta_performer.cc:152:
buffer_.erase(buffer_.begin(), buffer_.begin() + strlen(kDeltaMagic) +
On 2010/04/20 23:52:38, Daniel Erat wrote:
> can you use RemoveBufferHeadBytes() here instead?

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode204
src/platform/update_engine/delta_performer.cc:204: const
DeltaArchiveManifest_InstallOperation& operation) {
On 2010/04/20 23:52:38, Daniel Erat wrote:
> CHECK(operation.type() == DeltaArchiveManifest_InstallOperation_Type_REPLACE
||
> operation.type() == DeltaArchiveManifest_InstallOperation_Type_REPLACE_BZ)
> 
> (you check this later, but it'd probably be good to explicitly check it at the
> top of the method and then just use NOTREACHED() in the else section below)

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode225
src/platform/update_engine/delta_performer.cc:225: TEST_AND_RETURN_FALSE(false);
On 2010/04/20 23:52:38, Daniel Erat wrote:
> why not just "return false"?  the only additional thing that
> TEST_AND_RETURN_FALSE() will do is print a useless "false failed." error
message

obviated.

http://codereview.chromium.org/1718001/diff/1/9#newcode246
src/platform/update_engine/delta_performer.cc:246: // Calculate buffer size.
Note, this function doesn't do a slideing
On 2010/04/20 23:52:38, Daniel Erat wrote:
> s/slideing/sliding/

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode252
src/platform/update_engine/delta_performer.cc:252: for (int i = 0; i <
operation.src_extents_size(); i++) {
On 2010/04/20 23:52:38, Daniel Erat wrote:
> be consistent about whether single-line if/for/while statements have curly
> braces or not (ExtentsToBsdiffPositionsString() later in the file doesn't use
> braces)

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode267
src/platform/update_engine/delta_performer.cc:267: ssize_t bytes_read_iter = 0;
On 2010/04/20 23:52:38, Daniel Erat wrote:
> choose a better name?  "iter" makes me think it's an stl iterator

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode301
src/platform/update_engine/delta_performer.cc:301: for (int i = 0; i <
extents.size() && length < full_length; i++) {
On 2010/04/20 23:52:38, Daniel Erat wrote:
> 'i' should hit extents.size() at the same time that length hits full_length,
> right?  if so, why not only use the 'i < extents.size()' condition here, and
> then DCHECK_EQ(length, full_length) after the loop is done?  (it looks like
> you're already sorta checking the same thing in the DCHECK within the loop)

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode337
src/platform/update_engine/delta_performer.cc:337:
TEST_AND_RETURN_FALSE(utils::MakeTempFile("/tmp/au_patch.XXXXXX",
On 2010/04/20 23:52:38, Daniel Erat wrote:
> doesn't need to be part of this CL, but it'd be better if you made the temp
> directory be a flag or constant shared across all this code instead of
> hardcoding it in many places

it's on the list.

http://codereview.chromium.org/1718001/diff/1/10
File src/platform/update_engine/delta_performer.h (right):

http://codereview.chromium.org/1718001/diff/1/10#newcode38
src/platform/update_engine/delta_performer.h:38: // The length value of the last
extent in the string may be stort, since
On 2010/04/20 23:52:38, Daniel Erat wrote:
> s/stort/short/ ?

Done.

http://codereview.chromium.org/1718001/diff/1/10#newcode40
src/platform/update_engine/delta_performer.h:40: // Also, an extent starting at
kSpareseHole, appears as -1 in the string.
On 2010/04/20 23:52:38, Daniel Erat wrote:
> s/kSpareseHole,/kSparseHole/

Done.

http://codereview.chromium.org/1718001/diff/1/10#newcode41
src/platform/update_engine/delta_performer.h:41: // For example, if the Extents
are {1, 1}, {4, 2}, {kSpareseHole, 1},
On 2010/04/20 23:52:38, Daniel Erat wrote:
> s/kSpareseHole/kSparseHole/

Done.

http://codereview.chromium.org/1718001/diff/1/10#newcode46
src/platform/update_engine/delta_performer.h:46: uint64_t block_size,
On 2010/04/20 23:52:38, Daniel Erat wrote:
> include inttypes.h in this header instead of in the .cc file

Done.

http://codereview.chromium.org/1718001/diff/1/10#newcode47
src/platform/update_engine/delta_performer.h:47: uint64_t full_length);
On 2010/04/20 23:52:38, Daniel Erat wrote:
> nit: add a newline before 'private'

Done.

http://codereview.chromium.org/1718001/diff/1/10#newcode52
src/platform/update_engine/delta_performer.h:52: const
chromeos_update_engine::DeltaArchiveManifest_InstallOperation&
On 2010/04/20 23:52:38, Daniel Erat wrote:
> you shouldn't need the "chromeos_update_engine::"; you're already in this
> namespace

Done.

http://codereview.chromium.org/1718001/diff/1/10#newcode62
src/platform/update_engine/delta_performer.h:62: bool PerformCopyOperation(
On 2010/04/20 23:52:38, Daniel Erat wrote:
> why "Copy" instead of "Move" (to match the enum's name in the proto)?

old name. fixed.

http://codereview.chromium.org/1718001/diff/1/10#newcode78
src/platform/update_engine/delta_performer.h:78: // Data buffered from the
binary blobs section.
On 2010/04/20 23:52:38, Daniel Erat wrote:
> this comment isn't exactly correct, is it?  buffer_ contains the beginning of
> the update (that is, the protobuf) until you see all of the protobuf; then you
> erase the protobuf from the beginning and use it to store the blobs

changed.

http://codereview.chromium.org/1718001/diff/1/10#newcode81
src/platform/update_engine/delta_performer.h:81: uint64 buffer_offset_;
On 2010/04/20 23:52:38, Daniel Erat wrote:
> use uint64_t

Done.

http://codereview.chromium.org/1718001/diff/1/10#newcode84
src/platform/update_engine/delta_performer.h:84: uint32 block_size_;
On 2010/04/20 23:52:38, Daniel Erat wrote:
> use uint32_t

Done.

http://codereview.chromium.org/1718001/diff/1/10#newcode85
src/platform/update_engine/delta_performer.h:85: };
On 2010/04/20 23:52:38, Daniel Erat wrote:
> add DISALLOW_COPY_AND_ASSIGN(DeltaPerformer) (you should add this to other
> classes too -- the ones in file_writer.h are missing it, for instance)

Done.

http://codereview.chromium.org/1718001/diff/1/10#newcode88
src/platform/update_engine/delta_performer.h:88: const int kDeltaVersionLength =
8;
On 2010/04/20 23:52:38, Daniel Erat wrote:
> what are these doing down here?

Good question. Moved.

http://codereview.chromium.org/1718001/diff/1/11
File src/platform/update_engine/delta_performer_unittest.cc (right):

http://codereview.chromium.org/1718001/diff/1/11#newcode55
src/platform/update_engine/delta_performer_unittest.cc:55: expected_output +=
StringPrintf("%" PRIu64 ":", start * block_size);
On 2010/04/20 23:52:38, Daniel Erat wrote:
> i don't think that this test accomplishes much, since it contains essentially
> the same code as what it's testing.  it'd probably be better to hand-write the
> expected string by looking at the test data above, and then just compare
against
> that

Done.

http://codereview.chromium.org/1718001/diff/1/11#newcode110
src/platform/update_engine/delta_performer_unittest.cc:110: bool
WriteSpareFile(const string& path, off_t size) {
On 2010/04/20 23:52:38, Daniel Erat wrote:
> s/WriteSpareFile/WriteSparseFile/

Done.

http://codereview.chromium.org/1718001/diff/1/20
File src/platform/update_engine/utils.cc (right):

http://codereview.chromium.org/1718001/diff/1/20#newcode67
src/platform/update_engine/utils.cc:67: offset + bytes_read);
On 2010/04/20 23:52:38, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/1718001/diff/1/21
File src/platform/update_engine/utils.h (right):

http://codereview.chromium.org/1718001/diff/1/21#newcode188
src/platform/update_engine/utils.h:188: LOG(ERROR) << "Unable to remove dir " <<
path_ << ": " << err_message;
On 2010/04/20 23:52:38, Daniel Erat wrote:
> why not use PLOG(ERROR) instead so you don't need to call strerror()?

Done.

[Daniel Erat, 2010-04-23 17:47:25]

LGTM after a few changes

http://codereview.chromium.org/1718001/diff/1/9
File src/platform/update_engine/delta_performer.cc (right):

http://codereview.chromium.org/1718001/diff/1/9#newcode12
src/platform/update_engine/delta_performer.cc:12: #include "base/scoped_ptr.h"
On 2010/04/23 04:48:11, adlr wrote:
> On 2010/04/20 23:52:38, Daniel Erat wrote:
> > the include order here seems kinda weird -- move the "base/..." includes
into
> > the same block as the "update_engine/..." ones?
> 
> I thought base was a library. I certainly link a library called base and I
> thought this was the same code...
> 
> The order I have is:
> - corresponding .h
> - C standard includes
> - C++ standard includes
> - library includes
> - local includes

The style guide says this:

1. dir2/foo2.h (preferred location — see details below).
2. C system files.
3. C++ system files.
4. Other libraries' .h files.
5. Your project's .h files.

I've interpreted "your project's .h files" to mean "Chrome OS's .h files", that
is, stuff that has doublequotes around it instead of brackets, mostly since that
seems to be what Chrome does too (choosing a file at random):
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/gtk/browser_wi...

http://codereview.chromium.org/1718001/diff/1/9#newcode135
src/platform/update_engine/delta_performer.cc:135: memcpy(&protobuf_length,
On 2010/04/23 04:48:11, adlr wrote:
> On 2010/04/20 23:52:38, Daniel Erat wrote:
> > #include <cstring> for this
> 
> how about just <string.h>, as specified by man memcpy? If so, done.

I think that the C header string.h is deprecated in C++ in favor of the C++
wrapper cstring, but sure, that'll probably work too.

http://codereview.chromium.org/1718001/diff/9001/10008
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/1718001/diff/9001/10008#newcode115
src/platform/update_engine/delta_diff_generator.h:115: const char* const
kBsdiffPath = "/usr/bin/bsdiff";
Same objection as in delta_performer.h here -- I don't think that you should be
defining constants at the bottom of a header file (why at the bottom?  why in an
anon namespace?).

http://codereview.chromium.org/1718001/diff/9001/10024
File src/platform/update_engine/generate_delta_main.cc (right):

http://codereview.chromium.org/1718001/diff/9001/10024#newcode65
src/platform/update_engine/generate_delta_main.cc:65: CHECK_EQ(0,
performer.Open(FLAGS_old_image.c_str(), 0, 0));
mind using the more-natural-to-read CHECK_EQ(actual, expected) here?  i think
it's only the gtest EXPECT/ASSERT macros that take the expected first, and
that's just because they're copying junit or whatever

http://codereview.chromium.org/1718001/diff/9001/10034
File src/platform/update_engine/test_utils.h (right):

http://codereview.chromium.org/1718001/diff/9001/10034#newcode122
src/platform/update_engine/test_utils.h:122: const std::string dev_;
add DISALLOW_COPY_AND_ASSIGN (especially for RAII-ish classes)

http://codereview.chromium.org/1718001/diff/9001/10036
File src/platform/update_engine/utils.h (right):

http://codereview.chromium.org/1718001/diff/9001/10036#newcode178
src/platform/update_engine/utils.h:178: const std::string path_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/1718001/diff/9001/10036#newcode190
src/platform/update_engine/utils.h:190: const std::string path_;
DISALLOW_COPY_AND_ASSIGN

[adlr, 2010-04-23 20:50:17]

fixed and submitted, thanks

http://codereview.chromium.org/1718001/diff/1/9
File src/platform/update_engine/delta_performer.cc (right):

http://codereview.chromium.org/1718001/diff/1/9#newcode12
src/platform/update_engine/delta_performer.cc:12: #include "base/scoped_ptr.h"
On 2010/04/23 17:47:25, Daniel Erat wrote:
> On 2010/04/23 04:48:11, adlr wrote:
> > On 2010/04/20 23:52:38, Daniel Erat wrote:
> > > the include order here seems kinda weird -- move the "base/..." includes
> into
> > > the same block as the "update_engine/..." ones?
> > 
> > I thought base was a library. I certainly link a library called base and I
> > thought this was the same code...
> > 
> > The order I have is:
> > - corresponding .h
> > - C standard includes
> > - C++ standard includes
> > - library includes
> > - local includes
> 
> The style guide says this:
> 
> 1. dir2/foo2.h (preferred location — see details below).
> 2. C system files.
> 3. C++ system files.
> 4. Other libraries' .h files.
> 5. Your project's .h files.
> 
> I've interpreted "your project's .h files" to mean "Chrome OS's .h files",
that
> is, stuff that has doublequotes around it instead of brackets, mostly since
that
> seems to be what Chrome does too (choosing a file at random):
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/gtk/browser_wi...

Done.

http://codereview.chromium.org/1718001/diff/1/9#newcode135
src/platform/update_engine/delta_performer.cc:135: memcpy(&protobuf_length,
On 2010/04/23 17:47:25, Daniel Erat wrote:
> On 2010/04/23 04:48:11, adlr wrote:
> > On 2010/04/20 23:52:38, Daniel Erat wrote:
> > > #include <cstring> for this
> > 
> > how about just <string.h>, as specified by man memcpy? If so, done.
> 
> I think that the C header string.h is deprecated in C++ in favor of the C++
> wrapper cstring, but sure, that'll probably work too.

Didn't realize this. I've changed it and I'll try to remember to use the <c...>
headers in the future.

http://codereview.chromium.org/1718001/diff/9001/10008
File src/platform/update_engine/delta_diff_generator.h (right):

http://codereview.chromium.org/1718001/diff/9001/10008#newcode115
src/platform/update_engine/delta_diff_generator.h:115: const char* const
kBsdiffPath = "/usr/bin/bsdiff";
On 2010/04/23 17:47:25, Daniel Erat wrote:
> Same objection as in delta_performer.h here -- I don't think that you should
be
> defining constants at the bottom of a header file (why at the bottom?  why in
an
> anon namespace?).

Sorry, I think I just threw this in while writing code. It seemed like, at least
for integer constant, this might be more efficient since they could be inlined.
the anon namespace was the c++ conversion of 'static', which is how i would have
done it in c.

anyway, changed to non-anon namespace and to just declarations with the
definitions in the corresponding .cc file.

http://codereview.chromium.org/1718001/diff/9001/10024
File src/platform/update_engine/generate_delta_main.cc (right):

http://codereview.chromium.org/1718001/diff/9001/10024#newcode65
src/platform/update_engine/generate_delta_main.cc:65: CHECK_EQ(0,
performer.Open(FLAGS_old_image.c_str(), 0, 0));
On 2010/04/23 17:47:25, Daniel Erat wrote:
> mind using the more-natural-to-read CHECK_EQ(actual, expected) here?  i think
> it's only the gtest EXPECT/ASSERT macros that take the expected first, and
> that's just because they're copying junit or whatever

Done.

http://codereview.chromium.org/1718001/diff/9001/10034
File src/platform/update_engine/test_utils.h (right):

http://codereview.chromium.org/1718001/diff/9001/10034#newcode122
src/platform/update_engine/test_utils.h:122: const std::string dev_;
On 2010/04/23 17:47:25, Daniel Erat wrote:
> add DISALLOW_COPY_AND_ASSIGN (especially for RAII-ish classes)

Done.

http://codereview.chromium.org/1718001/diff/9001/10036
File src/platform/update_engine/utils.h (right):

http://codereview.chromium.org/1718001/diff/9001/10036#newcode178
src/platform/update_engine/utils.h:178: const std::string path_;
On 2010/04/23 17:47:25, Daniel Erat wrote:
> DISALLOW_COPY_AND_ASSIGN

Done (for all in this file)

http://codereview.chromium.org/1718001/diff/9001/10036#newcode190
src/platform/update_engine/utils.h:190: const std::string path_;
On 2010/04/23 17:47:25, Daniel Erat wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.