AU: Minor fixes to get it to do full update on real device

AU: Minor fixes to get it to do full update on real device

- Link w/ gtest only for unittests.

- filesystem_copier_action: always send install plan on the output pipe
  on success, even if no copy was performed.

- omaha_response_handler: properly choose install partition based on
  new GPT partition spec.

- More useful logging to match memento_updater (which update URL is
  used, dump of the request/response for the update check).

- Fixed a bug where I wasn't bonding the proper actions together in update_attempter.

BUG=None
TEST=attached unittests/did full update on Eee PC

[davidjames, 2010-05-06 19:54:51]

LGTM w/suggestions.

(DISCLAIMER: I am completely new to the update manager. If my suggestions are
not good ideas, please correct me appropriately.)

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

http://codereview.chromium.org/2044001/diff/1/7#newcode68
src/platform/update_engine/omaha_response_handler_action.cc:68: *it = (*it ==
'3') ? '5' : '3';
A few questions here.

1. Should we log an error when the boot device is empty and/or the partition
number is not '3' or '5'?
2. Should we do more checking to see if the boot device is sane?
3. Would this be more clear if we split it up into ordinary 'if' statements or a
'switch' statement?

E.g.
// Right now, we just switch '3' and '5' partition numbers.
if (*it == '3') {
  *it = '5';
} else if (*it == '5') {
  *it = '3';
} else {
  LOG(ERROR) << "Unrecognized boot device: " << boot_dev.c_str();
  return false;
}

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

http://codereview.chromium.org/2044001/diff/1/9#newcode79
src/platform/update_engine/update_attempter.cc:79:
BondActions(filesystem_copier_action.get(),
Looks like you added filesystem_copier_action to the list of actions here. Could
you update your log message to mention this?

http://codereview.chromium.org/2044001/diff/1/10
File src/platform/update_engine/update_check_action.cc (right):

http://codereview.chromium.org/2044001/diff/1/10#newcode197
src/platform/update_engine/update_check_action.cc:197: LOG(INFO) << "Update
check response: " << &response_buffer_[0];
Might it be more clear to use a string here? Adding a NULL byte to the vector
here might cause confusion over how long the string really is (because size() is
now changed and includes the null byte).

Here's a sketch of the same code using a string:

string respose_str(&response_buffer_[0], response_buffer.size());
LOG(INFO) << "Update check response: " << response_str;

[adlr, 2010-05-06 20:33:59]

Thanks. fixed and submitted

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

http://codereview.chromium.org/2044001/diff/1/7#newcode68
src/platform/update_engine/omaha_response_handler_action.cc:68: *it = (*it ==
'3') ? '5' : '3';
On 2010/05/06 19:54:52, davidjames wrote:
> A few questions here.
> 
> 1. Should we log an error when the boot device is empty and/or the partition
> number is not '3' or '5'?

Yes, we do via TEST_AND_RETURN_FALSE

> 2. Should we do more checking to see if the boot device is sane?

added some more checking per our verbal discussion

> 3. Would this be more clear if we split it up into ordinary 'if' statements or
a
> 'switch' statement?
> 
> E.g.
> // Right now, we just switch '3' and '5' partition numbers.
> if (*it == '3') {
>   *it = '5';
> } else if (*it == '5') {
>   *it = '3';
> } else {
>   LOG(ERROR) << "Unrecognized boot device: " << boot_dev.c_str();
>   return false;
> }

i think this is more a judgement call. I prefer the succinctness of the few
lines i have now, and also dan has in general encouraged me in this direction.

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

http://codereview.chromium.org/2044001/diff/1/9#newcode79
src/platform/update_engine/update_attempter.cc:79:
BondActions(filesystem_copier_action.get(),
On 2010/05/06 19:54:52, davidjames wrote:
> Looks like you added filesystem_copier_action to the list of actions here.
Could
> you update your log message to mention this?

Done.

http://codereview.chromium.org/2044001/diff/1/10
File src/platform/update_engine/update_check_action.cc (right):

http://codereview.chromium.org/2044001/diff/1/10#newcode197
src/platform/update_engine/update_check_action.cc:197: LOG(INFO) << "Update
check response: " << &response_buffer_[0];
On 2010/05/06 19:54:52, davidjames wrote:
> Might it be more clear to use a string here? Adding a NULL byte to the vector
> here might cause confusion over how long the string really is (because size()
is
> now changed and includes the null byte).
> 
> Here's a sketch of the same code using a string:
> 
> string respose_str(&response_buffer_[0], response_buffer.size());
> LOG(INFO) << "Update check response: " << response_str;

Done.