Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3)

Issue 5876066: mac: Implement GetMachineId(). (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 12 months ago by thakis
Modified:
10 years, 12 months ago
CC:
rlz-codereviews_googlegroups.com, Roger Tawa
Base URL:
https://rlz.googlecode.com/svn/trunk
Visibility:
Public.

Description

mac: Implement GetMachineId(). BUG=chromium:117739 TEST=none

Patch Set 1 #

Total comments: 25

Patch Set 2 : mark1 #

Patch Set 3 : scopedioobj #

Patch Set 4 : base::mac:: #

Total comments: 15

Patch Set 5 : . #

Patch Set 6 : comma #

Total comments: 9

Patch Set 7 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+133 lines, -1 line) Patch
M mac/lib/machine_id_mac.cc View 1 2 3 4 5 6 1 chunk +133 lines, -1 line 0 comments Download

Messages

Total messages: 14
thakis
10 years, 12 months ago (2012-03-23 00:06:30 UTC) #1
Roger Tawa (Google)
a couple of nits, but Mark should l g t m https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc File mac/lib/machine_id_mac.cc (right): ...
10 years, 12 months ago (2012-03-23 14:31:24 UTC) #2
Mark Mentovai
https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc File mac/lib/machine_id_mac.cc (right): https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newcode33 mac/lib/machine_id_mac.cc:33: return false; matching_dict is leaked in this case. Normally, ...
10 years, 12 months ago (2012-03-23 14:46:12 UTC) #3
thakis
https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc File mac/lib/machine_id_mac.cc (right): https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newcode15 mac/lib/machine_id_mac.cc:15: #include <IOKit/network/IOEthernetController.h> On 2012/03/23 14:31:24, rogerta wrote: > don't ...
10 years, 12 months ago (2012-03-23 15:27:56 UTC) #4
Mark Mentovai
Respond to this one again when it’s moved to ScopedIOObject and I’ll take another look
10 years, 12 months ago (2012-03-23 16:26:59 UTC) #5
thakis
Done, but ScopedIOObject is pretty clunky. while (ScopedIOObject<io_object_t> primary_interface = IOIteratorNext(primary_interface_iterator)) { ^ doesn't work, ...
10 years, 12 months ago (2012-03-23 16:35:46 UTC) #6
Mark Mentovai
https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc File mac/lib/machine_id_mac.cc (right): https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#newcode56 mac/lib/machine_id_mac.cc:56: for (base::mac::ScopedIOObject<io_object_t> primary_interface( Why not declare primary_interface outside of ...
10 years, 12 months ago (2012-03-23 19:01:10 UTC) #7
thakis
Thanks! https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc File mac/lib/machine_id_mac.cc (right): https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#newcode56 mac/lib/machine_id_mac.cc:56: for (base::mac::ScopedIOObject<io_object_t> primary_interface( On 2012/03/23 19:01:11, Mark Mentovai ...
10 years, 12 months ago (2012-03-23 19:13:20 UTC) #8
Mark Mentovai
https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc File mac/lib/machine_id_mac.cc (right): https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#newcode56 mac/lib/machine_id_mac.cc:56: for (base::mac::ScopedIOObject<io_object_t> primary_interface( thakis wrote: > On 2012/03/23 19:01:11, ...
10 years, 12 months ago (2012-03-23 19:32:10 UTC) #9
thakis
https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc File mac/lib/machine_id_mac.cc (right): https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#newcode56 mac/lib/machine_id_mac.cc:56: for (base::mac::ScopedIOObject<io_object_t> primary_interface( On 2012/03/23 19:32:10, Mark Mentovai wrote: ...
10 years, 12 months ago (2012-03-23 19:56:08 UTC) #10
thakis
https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc File mac/lib/machine_id_mac.cc (right): https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#newcode56 mac/lib/machine_id_mac.cc:56: for (base::mac::ScopedIOObject<io_object_t> primary_interface( On 2012/03/23 19:56:08, thakis wrote: > ...
10 years, 12 months ago (2012-03-23 19:56:59 UTC) #11
Mark Mentovai
I’d LG this mod the changes but the “since this is in a loop” question ...
10 years, 12 months ago (2012-03-23 20:15:07 UTC) #12
thakis
https://codereview.appspot.com/5876066/diff/14001/mac/lib/machine_id_mac.cc File mac/lib/machine_id_mac.cc (right): https://codereview.appspot.com/5876066/diff/14001/mac/lib/machine_id_mac.cc#newcode24 mac/lib/machine_id_mac.cc:24: // See http://developer.apple.com/library/mac/#technotes/tn1103/_index.html On 2012/03/23 20:15:08, Mark Mentovai wrote: ...
10 years, 12 months ago (2012-03-23 20:20:46 UTC) #13
Mark Mentovai
10 years, 12 months ago (2012-03-23 20:38:22 UTC) #14
LGTM

https://codereview.appspot.com/5876066/diff/14001/mac/lib/machine_id_mac.cc
File mac/lib/machine_id_mac.cc (right):

https://codereview.appspot.com/5876066/diff/14001/mac/lib/machine_id_mac.cc#n...
mac/lib/machine_id_mac.cc:78: mac_data_data, CFRangeMake(0,
kIOEthernetAddressSize), buffer);
thakis wrote:
> On 2012/03/23 20:15:08, Mark Mentovai wrote:
> > Since this is in a loop…
> > 
> > Is there ever a case where there’d be more than one primary interface that
the
> > iterator gives access to? If not, you should probably programmatically
assert
> > that we don’t pass through this loop body more than once.
> 
> I have no idea. I'd think not, but this is what the code that Apple published
> does, so I'd just keep the logic close to that? Especially that if this can
> happen, it probably happens only on a weird machine configuration.

I’m just worried about the order being nondeterministic in the event that
there’s more than one pass through the loop.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b