AU: Use Omaha ID rather than MAC address in delta updater

AU: Use Omaha ID rather than MAC address in delta updater

[Daniel Erat, 2009-12-09 06:44:10]

http://codereview.chromium.org/467051/diff/1/5
File src/platform/update_engine/omaha_request_prep_action.cc (right):

http://codereview.chromium.org/467051/diff/1/5#newcode17
src/platform/update_engine/omaha_request_prep_action.cc:17: const string
kOmahaIdPath = "/mnt/stateful_partition/etc/omaha_id";
can you make the "/mnt/stateful_partition" string just stored in one place?

http://codereview.chromium.org/467051/diff/1/5#newcode47
src/platform/update_engine/omaha_request_prep_action.cc:47: string
GuidFromData(const char data[16]) {
document this function's output in a comment

http://codereview.chromium.org/467051/diff/1/5#newcode48
src/platform/update_engine/omaha_request_prep_action.cc:48: string ret = "{";
not a big deal, but it'd be maybe be cleaner to pre-allocate the final size with
resize() and then use replace() to set characters in it.  maybe not due to the
dashes, though

http://codereview.chromium.org/467051/diff/1/5#newcode74
src/platform/update_engine/omaha_request_prep_action.cc:74: char buf[16];
make this 16 a const shared between this and GuidFromData

http://codereview.chromium.org/467051/diff/1/5#newcode77
src/platform/update_engine/omaha_request_prep_action.cc:77: ssize_t rc =
read(rand_fd, buf + bytes_read, sizeof(buf) - bytes_read);
seems like it might be useful to have a function that reads a passed-in number
of bytes from an fd in util.cc (but i'm fine with keeping this here as long as
you're careful to move it there if you end up needing similar code elsewhere
later)

http://codereview.chromium.org/467051/diff/1/5#newcode87
src/platform/update_engine/omaha_request_prep_action.cc:87: ScopedFdCloser
fd_closer(&fd);
probably better to move this above the (fd < 0) block; i think it's easier to
read this if the code that uses 'fd' comes right after you open it

http://codereview.chromium.org/467051/diff/1/5#newcode89
src/platform/update_engine/omaha_request_prep_action.cc:89: char buf[39] = {0}; 
// length of a GUID + NULL
get rid of this magic number too, if possible

http://codereview.chromium.org/467051/diff/1/5#newcode92
src/platform/update_engine/omaha_request_prep_action.cc:92: ssize_t rc =
read(fd, buf + bytes_read, sizeof(buf) - bytes_read - 1);
can you replace all this with a call to util::ReadFile()?

http://codereview.chromium.org/467051/diff/1/5#newcode98
src/platform/update_engine/omaha_request_prep_action.cc:98: *out_id = buf;
shouldn't you check that the correct number of bytes were read?

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

http://codereview.chromium.org/467051/diff/1/7#newcode119
src/platform/update_engine/omaha_request_prep_action_unittest.cc:119:
EXPECT_TRUE(IsValidGuid(out.user_id));
any reason that you're printing the id on failure for machine_id but not for
user_id?

http://codereview.chromium.org/467051/diff/1/7#newcode187
src/platform/update_engine/omaha_request_prep_action_unittest.cc:187:
EXPECT_TRUE(DoTest(false, &out1));
what's the reason for putting this inside of its own scope?

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

http://codereview.chromium.org/467051/diff/1/10#newcode31
src/platform/update_engine/utils.cc:31: O_WRONLY | O_CREAT | O_TRUNC,
fix indenting

[adlr, 2009-12-09 20:46:05]

http://codereview.chromium.org/467051/diff/1/5
File src/platform/update_engine/omaha_request_prep_action.cc (right):

http://codereview.chromium.org/467051/diff/1/5#newcode17
src/platform/update_engine/omaha_request_prep_action.cc:17: const string
kOmahaIdPath = "/mnt/stateful_partition/etc/omaha_id";
On 2009/12/09 06:44:10, Daniel Erat wrote:
> can you make the "/mnt/stateful_partition" string just stored in one place?

Done.

http://codereview.chromium.org/467051/diff/1/5#newcode47
src/platform/update_engine/omaha_request_prep_action.cc:47: string
GuidFromData(const char data[16]) {
On 2009/12/09 06:44:10, Daniel Erat wrote:
> document this function's output in a comment

Done.

http://codereview.chromium.org/467051/diff/1/5#newcode48
src/platform/update_engine/omaha_request_prep_action.cc:48: string ret = "{";
On 2009/12/09 06:44:10, Daniel Erat wrote:
> not a big deal, but it'd be maybe be cleaner to pre-allocate the final size
with
> resize() and then use replace() to set characters in it.  maybe not due to the
> dashes, though

changed to stringprintf

http://codereview.chromium.org/467051/diff/1/5#newcode74
src/platform/update_engine/omaha_request_prep_action.cc:74: char buf[16];
On 2009/12/09 06:44:10, Daniel Erat wrote:
> make this 16 a const shared between this and GuidFromData

Done.

http://codereview.chromium.org/467051/diff/1/5#newcode77
src/platform/update_engine/omaha_request_prep_action.cc:77: ssize_t rc =
read(rand_fd, buf + bytes_read, sizeof(buf) - bytes_read);
On 2009/12/09 06:44:10, Daniel Erat wrote:
> seems like it might be useful to have a function that reads a passed-in number
> of bytes from an fd in util.cc (but i'm fine with keeping this here as long as
> you're careful to move it there if you end up needing similar code elsewhere
> later)

good point

http://codereview.chromium.org/467051/diff/1/5#newcode87
src/platform/update_engine/omaha_request_prep_action.cc:87: ScopedFdCloser
fd_closer(&fd);
On 2009/12/09 06:44:10, Daniel Erat wrote:
> probably better to move this above the (fd < 0) block; i think it's easier to
> read this if the code that uses 'fd' comes right after you open it

Done.

http://codereview.chromium.org/467051/diff/1/5#newcode89
src/platform/update_engine/omaha_request_prep_action.cc:89: char buf[39] = {0}; 
// length of a GUID + NULL
On 2009/12/09 06:44:10, Daniel Erat wrote:
> get rid of this magic number too, if possible

Done.

http://codereview.chromium.org/467051/diff/1/5#newcode92
src/platform/update_engine/omaha_request_prep_action.cc:92: ssize_t rc =
read(fd, buf + bytes_read, sizeof(buf) - bytes_read - 1);
On 2009/12/09 06:44:10, Daniel Erat wrote:
> can you replace all this with a call to util::ReadFile()?

Done.

http://codereview.chromium.org/467051/diff/1/5#newcode98
src/platform/update_engine/omaha_request_prep_action.cc:98: *out_id = buf;
On 2009/12/09 06:44:10, Daniel Erat wrote:
> shouldn't you check that the correct number of bytes were read?

Done.

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

http://codereview.chromium.org/467051/diff/1/7#newcode119
src/platform/update_engine/omaha_request_prep_action_unittest.cc:119:
EXPECT_TRUE(IsValidGuid(out.user_id));
On 2009/12/09 06:44:10, Daniel Erat wrote:
> any reason that you're printing the id on failure for machine_id but not for
> user_id?

no, fixed

http://codereview.chromium.org/467051/diff/1/7#newcode187
src/platform/update_engine/omaha_request_prep_action_unittest.cc:187:
EXPECT_TRUE(DoTest(false, &out1));
On 2009/12/09 06:44:10, Daniel Erat wrote:
> what's the reason for putting this inside of its own scope?

probably stale code. fixed

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

http://codereview.chromium.org/467051/diff/1/10#newcode31
src/platform/update_engine/utils.cc:31: O_WRONLY | O_CREAT | O_TRUNC,
On 2009/12/09 06:44:10, Daniel Erat wrote:
> fix indenting

Done.

[Daniel Erat, 2009-12-09 23:09:33]

LGTM