Fix uuid.uuid1() core logic of uuid.getnode() needs refresh #72196
Comments
|
Short version: the five routines get_node() calls to get the MAC address either fail by definition, or is wrong. The one routine that works - is wrong because it returns the same value regardless of the system it runs on - wrong character is used to identify the string containing the mac address. Recommended: correct and call the one routine that can work for AIX. Details (using Python2.7 as template, applies to all Python3 versions as well) +511 def getnode(): unixdll_getnode depends on finding uuid_generate_time in either libuuid, libc, or None On a default install of AIX: +339 def _ifconfig_getnode(): Does not work on AIX - why call it? +347 def _arp_getnode(): Does not work on one Linux system I tried on AIX: +358 def _lanscan_getnode(): Again, from comments - looks like it should work on HP-UX, so why call it on AIX So, finally, after 4 guaranteed failures the following is called: +363 def _netstat_getnode(): For AIX - lines 380 and 381 do work - except the answer is ALWAYS the same: See host x071: See host x064: The answer found is always Where, for AIX at least, line 380 and line 381 should be looking at the character '.', not ':' So, two corrections suggested: line 380 + 381 (or as appropriate per Python version) - modify: +378 words = line.rstrip().split() NOTE: if str need to be a chr, then use that instead. But much earlier in program logic - modify: +525 if sys.platform == 'win32': to +529 if sys.platform == 'win32': |
aixtools
added
stdlib
|
+531 elif sys.platform.startswith("aix"): LGTM. Do you want to write this a pull request? |
|
_unixdll_getnode, _ifconfig_getnode, and _arp_getnode were changed recently. Are they still not working on AIX? |
|
On 11/20/2017 5:22 PM, Serhiy Storchaka wrote:
root@x071:[/root]nm -Ae /usr/lib/libc.a | grep uuid i.e., there is nothing with uuid_generate in it. _arp_getnode()
AIX does not return any value for itself.
Neither does Centos, (so I expect RHEL will also not), and my old
debians do not either.So, after summary - these three functions still do nothing/do not work And - as before, netstat -ia - when looking at ':', still returns 01:00:5e:00:00:01 for every interface.
|
|
What return commands |
|
x064 is AIX 5.3, x071 is AIX 6.1, x072 is AIX 7.1 - in the following output On 11/20/2017 9:01 PM, Serhiy Storchaka wrote:
ichael@x071:[/home/michael]dsh -n x064,x071,x072 ifconfig ifconfig -a: michael@x071:[/home/michael]dsh -n x064,x071,x072 "ifconfig -a" ifconfig -av: michael@x071:[/home/michael]dsh -n x064,x071,x072 "ifconfig -av" ip link list: arp -an: michael@x071:[/home/michael]dsh -n x064,x071,x072 "arp -an" x064: ? (192.168.129.254) at e0:28:6d:d6:eb:7b [ethernet] stored in x072: ? (192.168.129.254) at e0:28:6d:d6:eb:7b [ethernet] stored in lanscan -ai:
|
|
"LGTM. Do you want to write this a pull request?" Michael? You didn't answer to my question. |
|
Sure - I'll work on a PR. This will be the easier one. Where I am currently 'lost' is to correct _uuidmodule.c - another thing that has always been broken (new issue, about to open). |
|
Considering that _uuid is now working for AIX bpo-32399 - I need to get some things straight (aka some help please). Does uuid.py (./Lib/uuid.py) call _uuid.py? If not, I am curious how _uuid.c is used - because ./Lib/test/test_uuid.py does not seem to be successful in finding it:
So, for now it seems the test is only testing 'uuid.py'. I would like to see the added value of having gotten the _uuid Module to build. However, if this is 'working as designed' I not worry about it and just go back into the util.py - getnode() etc. |
|
On 08/01/2018 16:07, Michael Felt wrote:
Got this figured out. tested on too many machines - the last version Running on AIX6 - I get: (Pdb) py_uuid However, have a new question - why are py_uuid and c_uuid 'equivalent'. A comment - perhaps a new bpo/issue topic. When trying to run the tests michael@x071:[/data/prj/python/git/python3-master]python -m pdb
./Lib/test/test_uuid.py
Traceback (most recent call last):
File "/opt/lib/python2.7/pdb.py", line 1314, in main
pdb._runscript(mainpyfile)
File "/opt/lib/python2.7/pdb.py", line 1233, in _runscript
self.run(statement)
File "/opt/lib/python2.7/bdb.py", line 400, in run
exec cmd in globals, locals
File "<string>", line 1, in <module>
File "./Lib/test/test_uuid.py", line 520
print(hex, end=' ')
|
|
On 08/01/2018 17:22, Michael wrote:
some time differences - this is on POWER6 (10 years old system) - I michael@x071:[/data/prj/python/git/xlc-python3-3.7]./python michael@x071:[/data/prj/python/git/xlc-python3-3.7]python The second example is a) before the _uuid support was in the code, and The relative difference is: 911/2860 :: 31.85/100 - or roughly 3x faster As far as this issue is concerned - with _uuid active the buggy code I Question: (must have questions!) - the behavior of getnode() without
|
I'm also certain getnode() is supposed to return a hardware-determined number (usually the MAC address of an Ethernet interface). A random output wouldn't need so much platform-specific harness. |
|
On 09/01/2018 16:21, Antoine Pitrou wrote:
FYI The current implementation (Lib/uuid.py) - python2 as an example with Program (x2.py): michael@x071:[/data/prj/python/git]cat *master/x2.py michael@x071:[/data/prj/python/git]python *master/x2.py getnode = a92929dc8a78michael@x071:[/data/prj/python/git]python *master/x2.py getnode = cdff09dd1306So it appears, everytime python runs - without _uuid - a new "getnode()" Below - the value returned is the MAC address of the first ethernet michael@x071:[/data/prj/python/git]gcc*/python *master/x2.py getnode = fad18cf76204michael@x071:[/data/prj/python/git]xlc*/python *master/x2.py getnode = fad18cf76204Is there any interest for a PR on Python2.7 and/or Python3.5? I guess it HTH, Michael
|
|
PR added, please review. |
|
FYI: - On AIX, no uuid_create support: ./python -m unittest -v test.test_uuid OK (skipped=37) On AIX 6.1 (with ctypes, without _uuid) ./python -m unittest -v test.test_uuid OK (skipped=32) On AIX 6.1 (with ctypes, with _uuid) ./python -m unittest -v test.test_uuid OK (skipped=16) +++ nohup ./python -m unittest -v test.test_uuid | grep skipped |
|
Is nohup required in the example you just posted or is that a red herring? |
|
On 14/01/2018 22:01, Antoine Pitrou wrote:
nohup is required (i.e., my lazy way!) to stream both stdout and stderr Working on the Windows issue (my mistake). Will be groping in the dark |
|
Please review PR. at least for "master" |
|
OK. as promised when I closed PR 5183 - a restart. You may skip the wall that follows - it is just documentation. The key points:
A minor point: My question - getting back to what I had done previously - even though _uuid is working, should the "WithoutExtModule" and support for test_find_mac() be adjusted in uuid.py - OR - are these errors in test just going to be accepted asis? Details: (skip the wall :p ) So, with only test_uuid.py patched, the verbose returns ('ok' tests not included!): root@x066:[/data/prj/python/python3-3.8]./python ../git/**3.8/Lib/test/regrtest.py -v test_uuid | egrep -v "ok$" ====================================================================== Traceback (most recent call last):
File "/data/prj/python/git/python3-3.8/Lib/test/test_uuid.py", line 548, in test_find_mac
self.assertEqual(mac, 0x1234567890ab)
AssertionError: None != 20015998341291====================================================================== Traceback (most recent call last):
File "/data/prj/python/git/python3-3.8/Lib/test/test_uuid.py", line 548, in test_find_mac
self.assertEqual(mac, 0x1234567890ab)
AssertionError: None != 20015998341291Ran 50 tests in 0.629s FAILED (failures=2, skipped=23) == Tests result: FAILURE == 1 test failed: Total duration: 797 ms |
|
@pitrou: re: Is nohup required in the example you just posted or is that a red herring? I just use nohup to merge stdout and stderr when grepping for messages - and to, ideally, have the stdout and stderr messages properly synced in the output stream. So, I hope "red herring" :) My apologies for the belated reply. |
|
re; the current status of PR8672 - which I shall probably close as it no longer merges. @taleinat re: the need for lambda As _find_mac_netstat() is only called once the need for the last two arguments may be unnecessary. My reason for including them was to keep _find_mac_netstat comparable to _find_mac.
Linux: ifconfig (keyword "ether") root@x074:~# ifconfig So, when the keyword is found, as each word on the line is examined "i+1" gives the value For AIX (in anycase), the keyword is Address - the output is: Note - the value with colons - as many (all?) other systems - this seems to be a constant, and a value on a line all by itself - so the old code could never ever find it, even if it could have parsed it. The actual MAC address is on a line with several entries - matching the values given by the "header" line - so lambda i: i is needed to examine the later lines to find a suitably formatted value. So, should I write _find_mac_netstat for AIX only (and maybe set "skipIf" Linux). There are many assumptions in this code. I do not feel qualified to change things I cannot test - so, as much as possible I follow the tried and true. I hope this clarifies my intent well enough that you can make a decision. |
|
Historically, I started this issue because I saw that none of the calls made in uuid.py were working for AIX. I also assumed that they ALL worked already, at least somewhere. a) one cause is the difference between AIX and (all) others was the letter chosen to separate the six fields that from the hexadecimal macaddr. (AIX uses '.', others use ':'). b) on AIX only netstat -ia returns a macaddr - so the GETTERS array of routines to call could be shorter c) as the tests accept (I'll call it) no response aka None this was "passing" for all calls on AIX, including the mock test which had ":" hard coded into the test procedure and the mock data. d) the current test has to fail (i.e., always return None) when the output is coming from netstat because the output layout is fundamentally different. With netstat the keyword is in the first-line only and it determines the position, or index, of a potential value in the following lines. Other commands, such as ifconfig on linux, have the keyword and value on the same line with the keyword (position:index) proceeding the value (position:index+1) All this time I thought this was ONLY and AIX issue but in examining other platforms I see that some (e.g., Linux) do not have a macaddr value in netstat output - while others do (e.g., macos). In short, I would appreciate the current PR being merged so that AIX has at least basic support. However, I am ready to continue, as a new PR, re-working the tests (so that a response of None is at least a warning) when a macaddr is not found for a platform - rather than over customization of test_uuid.py (as I have started to do here for AIX). For platforms that do return a macaddr using netstat I would like to hear if that platform uses the "inline" method such as the command 'ifconfig' does on Linux, or if it is the "header" method such as on AIX and macos. Thank you for your consideration. |
aixtools
changed the title
|
As I am not clear on where to have a more general discussion (in a PR conversation) or here - going to start here because I cannot figure out which comment in the PR to reply to. Generally, before modifying the test_uuid.py to based tests on uuid.__NODE_GETTERS - these need to be defined. I have my AIX systems, I found a macos I could do some queries on, and downloaded cygwin and came up with this starting point: _MACOS = sys.platform == 'darwin'
_WIN32 = sys.platform == 'win32'
_CYGWIN= sys.platform == 'cygwin'
_AIX = sys.platform.startswith("aix")... if _AIX:
_NODE_GETTERS = [_unix_getnode, _netstat_getnode]
elif _MACOS:
_NODE_GETTERS = [_unix_getnode, _ifconfig_getnode, _netstat_getnode]
elif _CYGWIN:
_NODE_GETTERS = [_ipconfig_getnode]
elif _WIN32:
_NODE_GETTERS = [_windll_getnode, _ipconfig_getnode, _netbios_getnode]
else:
_NODE_GETTERS = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]What I am also wondering - is it worthwhile to have a way to only define the getter() routines a platform can actually use? e.g., On AIX I can call uuid._ipconfig_getter(), but get nonsense. Or is it too much effort? Finally, can someone with access to other platforms where differences may be expected (e.g., Solaris, hpux, or even different flavors of Linux) - to make this _NODE_GETTERS mode complete (specific). |
|
p.s., removed 2.7 and 3.6 as too old for any interest. |
|
The current code and proposed changes use 'netstat -ia' to find the node however if netstat needs to perform a reverse DNS query to resolve some interfaces this makes using uuid1 *really* slow especially when reverse DNS queries aren't set up correctly or timeout. Would it be possible to change the netstat call to add 'n' to the nestat options, i.e. _find_mac_netstat('netstat', '-ian', ...) to prevent netstat from attempting to resolve addresses? |
|
On 14/02/2019 23:57, Indra Talip wrote:
|
|
I have modified - _NODE_GETTERS_WIN32 = [_windll_getnode, _netbios_getnode, _ipconfig_getnode]
_NODE_GETTERS_UNIX = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]to: +683 # _OS_GETTERS, when known, are targetted for a specific OS or platform. The shortened list, and in particular the move of _ip_getnode before _ifconfig_getnode is my experience that the "old" programs such as ifconfig, arp, and netstat are (occasionally) not available - with "ip" being the replacement for all. Further, re: linux, on the two distros I could test (centos and debian) neither arp nor netstat return a (useable) MACADDR aka "node" value. Requesting verification from people with other platforms. Also, would like to know specifics for other platforms (e.g., OpenBSD, HPUX, Solaris). More generally speaking - if os.name is "posix" or "windows" - this lists are almost irrelevant because the "DLL" _uuid module should provide the needed value. The "plus" is that on systems that audit such things, there are fewer calls to non-existent programs and/or negative side-effects from calling programs that can/do not provide any useful data. |
|
p.s. - changed the title: way back when I first started on this I was mainly concerned that the _netstat_getnode() routine was broken for AIX. During the research and follow-up discussions it has become clear that it is more than just an AIX issue. There are multiple aspects that need attention. Footnote: For most platforms, most of the data accessed via Lib/uuid is actually retrieved via Modules/_uuid. The majority of issues with Lib/uuid occur during testing: ./python -m test test_uuid At least two PR (8672 - fix bug for AIX), (12777 - make "_getters" lists platform specific when possible). FYI: the first PR (5183) was when I was trying to patch multiple issues. |
aixtools
changed the title
I don't see this in PR #72196; should this change be made? |
On 14/07/2019 22:28, Tal Einat wrote:
> Tal Einat <taleinat@gmail.com> added the comment:
>
>>> The current code and proposed changes use 'netstat -ia' to find the node however if netstat needs to perform a reverse DNS query to resolve some interfaces this makes using uuid1 *really* slow especially when reverse DNS queries aren't set up correctly or timeout.
>>> mac = _find_mac_netstat('netstat', '-ia', b'Address', lambda i: i)
>>>
>>> Would it be possible to change the netstat call to add 'n' to the nestat options, i.e. _find_mac_netstat('netstat', '-ian', ...) to prevent netstat from attempting to resolve addresses?
>> Sounds like an excellent idea. Gives me a reason to get started again. Thx!
> I don't see this in PR python/cpython#72196; should this change be made?
I am testing with "-ian" - on top of the changes you have already made.
I'll push asap, assuming all goes well.
>
>
|
|
Michael, many many thanks for the time and effort that you have poured into getting this fixed, and for your patience and perseverance over the three years that it took to happen. Well done! |
taleinat
changed the title
|
I'll take a look as well. On 17/03/2020 16:14, STINNER Victor wrote:
|
|
I may be mistaken, but I do not think the change introduced a regression. While it is true that this case would not have appeared if there was If anyone is interested I will think about what are assumptions are - is IMHO - while bpo-39991 is resolved - I am not -yet- convinced that the Michael On 17/03/2020 16:14, STINNER Victor wrote:
|
I'm talking about this: I don't want to blame anyone. My intent here is to get more eyes on the changes that I merged in bpo-39991 to make sure that I didn't break any existing cases, and that I covered all cases.
Right, I pushed a second fix to also handle this case: commit ebf6bb9.
If you still see cases which are not handled properly with commit ebf6bb9, feel free to reopen bpo-39991. |
|
On 18/03/2020 13:55, STINNER Victor wrote:
I meant - I had never considered IPv6 in the Address column, just as I Your feedback made me realize that something like "fe80::78:9a:de:f0"
I will look closely at PR19045 - not because I expect to find anything Regards,
|
I cannot run functional tests on AIX. I can only rely on unit tests which contains a dump of AIX commands. That's why a review wouldn't hurt ;-) |
|
Your fix LGTM, Victor. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: