Issue 5876066: mac: Implement GetMachineId(). (Closed)
|
Created:
13 years, 1 month ago by thakis Modified:
13 years, 1 month ago CC:
rlz-codereviews_googlegroups.com, Roger Tawa Base URL:
https://rlz.googlecode.com/svn/trunk Visibility:
Public. |
Descriptionmac: 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 : . #MessagesTotal messages: 14
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): https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:15: #include <IOKit/network/IOEthernetController.h> don't system includes go before local includes? https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:39: CFRelease(primary_interface); indents above and a few other places below, like lines 72, 75, 126
Sign in to reply to this message.
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#newco... mac/lib/machine_id_mac.cc:33: return false; matching_dict is leaked in this case. Normally, IOServiceGetMatchingServices consumes the reference, but if you never call that, it can’t. You can use a ScopedCFTypeRef and call .release() when you call IOServiceGetMatchingServices. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:39: CFRelease(primary_interface); Again, putting primary_interface in a ScopedCFTypeRef and using .release() here is safer. The code now is correct, but not resistant to people adding early returns in the future. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:44: return kernResult == KERN_SUCCESS; Use C++-style naming rules: kern_result. Throughout this file. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:49: io_object_t intfService; This is a terrible name. Also, it’s formattedIncorrectly instead_of_this_way. Formatting on the next line, too. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:50: io_object_t controllerService; Don’t declare this until you need it. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:51: kern_return_t kernResult = KERN_FAILURE; This too. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:59: while ((intfService = IOIteratorNext(primary_interface_iterator))) { Use ScopedIOObject in here. It’s in chrome/browser/mac but you can move it out to base/mac. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:60: CFTypeRef mac_data; Don’t declare this until you need it. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:74: CFDataGetBytes((CFDataRef)mac_data, Don’t do unsafe casts like this when you’re unsure of the type. Use CFCast<> from base/foundation_util.h. (Don’t use C-style casts at all anyway.) https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:79: (void)IOObjectRelease(controllerService); (void) unnecessary. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:97: { OK, I’m stopping here. Please fix this file’s style to de-Apple it and conform to Google guidelines.
Sign in to reply to this message.
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#newco... mac/lib/machine_id_mac.cc:15: #include <IOKit/network/IOEthernetController.h> On 2012/03/23 14:31:24, rogerta wrote: > don't system includes go before local includes? Done. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:33: return false; On 2012/03/23 14:46:12, Mark Mentovai wrote: > matching_dict is leaked in this case. > > Normally, IOServiceGetMatchingServices consumes the reference, but if you never > call that, it can’t. > > You can use a ScopedCFTypeRef and call .release() when you call > IOServiceGetMatchingServices. Done. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:39: CFRelease(primary_interface); On 2012/03/23 14:46:12, Mark Mentovai wrote: > Again, putting primary_interface in a ScopedCFTypeRef and using .release() here > is safer. The code now is correct, but not resistant to people adding early > returns in the future. Done. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:39: CFRelease(primary_interface); On 2012/03/23 14:31:24, rogerta wrote: > indents above and a few other places below, like lines 72, 75, 126 Done. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:44: return kernResult == KERN_SUCCESS; On 2012/03/23 14:46:12, Mark Mentovai wrote: > Use C++-style naming rules: kern_result. Throughout this file. I tried! Done. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:49: io_object_t intfService; On 2012/03/23 14:46:12, Mark Mentovai wrote: > This is a terrible name. Also, it’s formattedIncorrectly instead_of_this_way. > Formatting on the next line, too. Done. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:50: io_object_t controllerService; On 2012/03/23 14:46:12, Mark Mentovai wrote: > Don’t declare this until you need it. Done. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:51: kern_return_t kernResult = KERN_FAILURE; On 2012/03/23 14:46:12, Mark Mentovai wrote: > This too. Done. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:59: while ((intfService = IOIteratorNext(primary_interface_iterator))) { On 2012/03/23 14:46:12, Mark Mentovai wrote: > Use ScopedIOObject in here. It’s in chrome/browser/mac but you can move it out > to base/mac. Will do. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:60: CFTypeRef mac_data; On 2012/03/23 14:46:12, Mark Mentovai wrote: > Don’t declare this until you need it. Done. https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:74: CFDataGetBytes((CFDataRef)mac_data, On 2012/03/23 14:46:12, Mark Mentovai wrote: > Don’t do unsafe casts like this when you’re unsure of the type. Use CFCast<> > from base/foundation_util.h. > > (Don’t use C-style casts at all anyway.) I looked at this, and from what I can tell my code would have to look like CFDataRef mac_cf_data = CFCast<CFDataRef>(mac_data); if (mac_rf_data) { // .. as before, with explicit release } I.e. CFCast<> doesn't mix well with scoped_cftyperef<>, and one can't just write scoped_cftyperef<CFDataRef> mac_data(CFCast<CFDataRef>(IORECCFP(...)); because that'd leak if IORECCFP() returns something of a different type. And since I need to have the explicit CFRelease() and everything anyway, maybe just if (mac_data && CFGetTypeId(mac_data) == CFStringGetTypeID) { // ... as before, but with static_cast } is shorter / simpler? https://codereview.appspot.com/5876066/diff/1/mac/lib/machine_id_mac.cc#newco... mac/lib/machine_id_mac.cc:97: { On 2012/03/23 14:46:12, Mark Mentovai wrote: > OK, I’m stopping here. Please fix this file’s style to de-Apple it and conform > to Google guidelines. The same CFCast<> argument I made above applies to the last few lines of this function, too.
Sign in to reply to this message.
Respond to this one again when it’s moved to ScopedIOObject and I’ll take another look
Sign in to reply to this message.
Done, but ScopedIOObject is pretty clunky.
while (ScopedIOObject<io_object_t> primary_interface =
IOIteratorNext(primary_interface_iterator)) {
^ doesn't work, because the io_object_t constructor is explicit.
while (ScopedIOObject<io_object_t> primary_interface =
ScopedIOObject<io_object_t>(IOIteratorNext(primary_interface_iterator)) {
^ doesn't work, no copy constructor
kern_return_t kern_result = IORegistryEntryGetParentEntry(
primary_interface, kIOServicePlane, &primary_interface_parent.get());
^ doesn't work, get() doesn't return a reference to the io_object_t
Sign in to reply to this message.
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#ne... mac/lib/machine_id_mac.cc:56: for (base::mac::ScopedIOObject<io_object_t> primary_interface( Why not declare primary_interface outside of the loop and write |while (primary_interface.reset(xxx), primary_interface)| ? https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:70: CFTypeRef mac_data = IORegistryEntryCreateCFProperty( What was wrong with a ScopedCFTypeRef here? You could do ScopedCFTypeRef<CFTypeRef> mac_data = …; CFDataRef mac_data_data = CFCast<CFDataRef>(mac_data); https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:99: CFTypeRef serial_number_cfstring = IORegistryEntryCreateCFProperty( Same thing here. It’s a two-liner, but so what? That’s still fewer lines than you’ve written. https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:117: *data = string16(); Why not data->clear()? https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:119: *data += ASCIIToUTF16(StringPrintf("%02x%02x%02x-%02x%02x%02x", Odd to see a MAC address formatted this way. Are you just matching the format that Windows RLZ uses?
Sign in to reply to this message.
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#ne... 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 wrote: > Why not declare primary_interface outside of the loop and write |while > (primary_interface.reset(xxx), primary_interface)| ? Ah, I tried that too, forgot to mention it. reset() returns void. https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:70: CFTypeRef mac_data = IORegistryEntryCreateCFProperty( On 2012/03/23 19:01:11, Mark Mentovai wrote: > What was wrong with a ScopedCFTypeRef here? > > You could do > > ScopedCFTypeRef<CFTypeRef> mac_data = …; > CFDataRef mac_data_data = CFCast<CFDataRef>(mac_data); Done. (Meh.) https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:99: CFTypeRef serial_number_cfstring = IORegistryEntryCreateCFProperty( On 2012/03/23 19:01:11, Mark Mentovai wrote: > Same thing here. It’s a two-liner, but so what? That’s still fewer lines than > you’ve written. Here I also have to release() the ScopedCFTypeRef before returning, but the return the casted variable. I think that's more complicated. Done. https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:117: *data = string16(); On 2012/03/23 19:01:11, Mark Mentovai wrote: > Why not data->clear()? Done. https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:119: *data += ASCIIToUTF16(StringPrintf("%02x%02x%02x-%02x%02x%02x", On 2012/03/23 19:01:11, Mark Mentovai wrote: > Odd to see a MAC address formatted this way. Are you just matching the format > that Windows RLZ uses? The formatting doesn't matter, this string gets sha1-d before anything is done with it. The only one who sees this string is people who add debug printfs to this function. Which formatting would you use?
Sign in to reply to this message.
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#ne... 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, Mark Mentovai wrote: > > Why not declare primary_interface outside of the loop and write |while > > (primary_interface.reset(xxx), primary_interface)| ? > > Ah, I tried that too, forgot to mention it. reset() returns void. That’s why I put the |, primary_interface| on the end. It’s OK to use that. https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:119: *data += ASCIIToUTF16(StringPrintf("%02x%02x%02x-%02x%02x%02x", On 2012/03/23 19:13:20, thakis wrote: > On 2012/03/23 19:01:11, Mark Mentovai wrote: > > Odd to see a MAC address formatted this way. Are you just matching the format > > that Windows RLZ uses? > > The formatting doesn't matter, this string gets sha1-d before anything is done > with it. The only one who sees this string is people who add debug printfs to > this function. > > Which formatting would you use? My only suggestion would be to use something to guarantee that if you don’t get a MAC address, there would be no collision with a serial number that might look like a MAC address. So I’d format it as "mac=%012x serial=%s" (yeah, I know you can’t really do 012x here, I’m just using it demonstratively.)
Sign in to reply to this message.
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#ne... 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: > thakis wrote: > > On 2012/03/23 19:01:11, Mark Mentovai wrote: > > > Why not declare primary_interface outside of the loop and write |while > > > (primary_interface.reset(xxx), primary_interface)| ? > > > > Ah, I tried that too, forgot to mention it. reset() returns void. > > That’s why I put the |, primary_interface| on the end. It’s OK to use that. O_O …done. https://codereview.appspot.com/5876066/diff/6002/mac/lib/machine_id_mac.cc#ne... mac/lib/machine_id_mac.cc:119: *data += ASCIIToUTF16(StringPrintf("%02x%02x%02x-%02x%02x%02x", On 2012/03/23 19:32:10, Mark Mentovai wrote: > On 2012/03/23 19:13:20, thakis wrote: > > On 2012/03/23 19:01:11, Mark Mentovai wrote: > > > Odd to see a MAC address formatted this way. Are you just matching the > format > > > that Windows RLZ uses? > > > > The formatting doesn't matter, this string gets sha1-d before anything is done > > with it. The only one who sees this string is people who add debug printfs to > > this function. > > > > Which formatting would you use? > > My only suggestion would be to use something to guarantee that if you don’t get > a MAC address, there would be no collision with a serial number that might look > like a MAC address. So I’d format it as "mac=%012x serial=%s" (yeah, I know you > can’t really do 012x here, I’m just using it demonstratively.) Done.
Sign in to reply to this message.
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#ne... 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: > On 2012/03/23 19:32:10, Mark Mentovai wrote: > > thakis wrote: > > > On 2012/03/23 19:01:11, Mark Mentovai wrote: > > > > Why not declare primary_interface outside of the loop and write |while > > > > (primary_interface.reset(xxx), primary_interface)| ? > > > > > > Ah, I tried that too, forgot to mention it. reset() returns void. > > > > That’s why I put the |, primary_interface| on the end. It’s OK to use that. > > O_O > > …done. (I like it, by the way. Just didn't feel like the google way to me, but you're the one who gets to decide that :-) )
Sign in to reply to this message.
I’d LG this mod the changes but the “since this is in a loop” question might have an answer that results in much code shifting. 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:24: // See http://developer.apple.com/library/mac/#technotes/tn1103/_index.html Document here that the caller assumes ownership of matching_services. https://codereview.appspot.com/5876066/diff/14001/mac/lib/machine_id_mac.cc#n... mac/lib/machine_id_mac.cc:54: bool success = true; Don’t you want to start with this at false so that if you never enter the loop, the function has the proper return value? 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); 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. https://codereview.appspot.com/5876066/diff/14001/mac/lib/machine_id_mac.cc#n... mac/lib/machine_id_mac.cc:91: (void)IOObjectRelease(primary_interface_iterator); Drop the (void) or use a scoper on primary_interface_iterator to make this exception-safe. :)
Sign in to reply to this message.
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:24: // See http://developer.apple.com/library/mac/#technotes/tn1103/_index.html On 2012/03/23 20:15:08, Mark Mentovai wrote: > Document here that the caller assumes ownership of matching_services. Done. https://codereview.appspot.com/5876066/diff/14001/mac/lib/machine_id_mac.cc#n... mac/lib/machine_id_mac.cc:54: bool success = true; On 2012/03/23 20:15:08, Mark Mentovai wrote: > Don’t you want to start with this at false so that if you never enter the loop, > the function has the proper return value? Done. 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); 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. https://codereview.appspot.com/5876066/diff/14001/mac/lib/machine_id_mac.cc#n... mac/lib/machine_id_mac.cc:91: (void)IOObjectRelease(primary_interface_iterator); On 2012/03/23 20:15:08, Mark Mentovai wrote: > Drop the (void) or use a scoper on primary_interface_iterator to make this > exception-safe. :) Done.
Sign in to reply to this message.
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.
|
