bpo-32199: uuid.getnode() now returns the MAC address on Android #4696
Conversation
Lib/uuid.py
Outdated
| @@ -401,7 +401,8 @@ def _ifconfig_getnode(): | |||
| def _ip_getnode(): | |||
| """Get the hardware address on Unix by running ip.""" | |||
| # This works on Linux with iproute2. | |||
| mac = _find_mac('ip', 'link list', [b'link/ether'], lambda i: i+1) | |||
| arg = 'link' if hasattr(sys, 'getandroidapilevel') else 'link list' | |||
There was a problem hiding this comment.
Can you please leave a comment explaining why link must be used instead of link list on Android? Even a URL would help avoid having to search for the underlying difference.
Also, I guess the new sys.getandroidapilevel attribute in 3.7 is the most reliable way to check for the Android platform? E.g. why is sys.platform not good enough?
There was a problem hiding this comment.
The ip link help command on linux and the ip man page on linux as well do not mention any list parameter for this command, so the question should be reversed, why list is being used here ? Probably some other platform using another version of iproute.
sys.platform does not list Android.
There was a problem hiding this comment.
On my Debian machine, I get the same output for both ip link and ip link list. If it's there to support an older CLI API for the ip link command, then why not just use that unconditionally? We'd have to watch the buildbots to see if any supported platform breaks. Simpler and less inscrutable code is better.
There was a problem hiding this comment.
_ip_getnode was added in issue 22902.
The ip command is part of the ìproute2 collection of network tools whose documentation states that the syntax is:
ip [ OPTIONS ] OBJECT [ COMMAND [ ARGUMENTS ]]
and that If no command is given a default command is assumed. The default command is usually show (list) but if the objects of the class cannot be listed then the default is to print out the command syntax help.
So it seems you are right and I will change the code to use only ip link in all cases.
There was a problem hiding this comment.
If this works on OS X, it LGTM.
There was a problem hiding this comment.
This looks as unneeded complication to me. All modern Linux distributions have either ifconfig, ip or arp (or all of them). If there was a version of ip that didn't support ip link, I suppose it was many years ago (you can check the sources of iproute2 to be sure: https://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git).
There was a problem hiding this comment.
I still think it's a mildly bad idea to just swallow the getter errors with no notification. @xdegaye maybe you can propose an optional API to turn on debugging here?
There was a problem hiding this comment.
@serhiy-storchaka packages such as iproute2 may be patched by a linux distribution and if they did not and we could just check for the source of iproute2, then there would not be any issue in the first place as Android (a linux distribution in this context) would have a functional ip link list then.
One may say, as it has been done before for the same kind of problem let us just use 'ip link' for now, because it is such an obvious change, and see if any one complain. But strictly speaking we cannot, as here the behavioral change that could trigger a complaint is a change in a single bit in the value returned by getnode().
@warsaw I agree it would be better to try to improve this with some kind of notification, maybe in another issue and we can leave this one pending meanwhile ?
There was a problem hiding this comment.
@xdegaye I'd personally prefer addressing the notification issue first so that we can get a sense of what's actually happening on user's machines, then come back and see if we can just use ip link for everything. OTOH, I think it's probably safe for the platforms we care about, and won't cause any behavior change in practice. But your change in this PR is admittedly the more conservative approach. I just don't like special casing Android here.
Here's another thought: what if you just added another getter that calls ip link list and placed that after one that calls ip link. Wouldn't that accomplish both goals? Then if ip link fails, we fall back to the old behavior, so nothing changes. It's uglier, but it doesn't special case for the Android platform, and eventually we can decide to remove ip link list altogether.
There was a problem hiding this comment.
Let discuss the design on the tracker.
|
When you're done making the requested changes, leave the comment: |
| @@ -0,0 +1 @@ | |||
| The getnode() ip getter now uses 'ip link'. | |||
There was a problem hiding this comment.
"instead of 'ip link list'"
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @warsaw: please review the changes made to this pull request. |
|
Thanks @xdegaye for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
…link list' (pythonGH-4696) (cherry picked from commit 961dbe0)
|
GH-4747 is a backport of this pull request to the 3.6 branch. |
https://bugs.python.org/issue32199