Skip to content

bpo-28009: Fix uuid.uuid1() and uuid.get_node() on AIX #8672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 43 commits into from
Sep 26, 2019
Merged

bpo-28009: Fix uuid.uuid1() and uuid.get_node() on AIX #8672

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
4ed41e9
uuid: correct the field seperator value of ethernet adapter MAC addre…
aixtools Aug 4, 2018
ba47933
Merge branch 'master' into bpo-28009-1
aixtools Aug 4, 2018
c6029a4
correct test logic, cut/paste error
aixtools Aug 4, 2018
7e1874d
specify shorter getters list per https://bugs.python.org/issue28009#m…
aixtools Aug 5, 2018
6a5bccb
write _notAIX as non-negative constant and use 'not AIX'
aixtools Aug 22, 2018
a98309b
simplify NEWS entry
aixtools Aug 22, 2018
53ef750
skip a test the right way
aixtools Aug 22, 2018
6fd5f8e
capitalize constants
aixtools Aug 22, 2018
3bb1c08
oops - restore missing assignment
aixtools Aug 22, 2018
1af8e3d
skip both tests the right way
aixtools Aug 22, 2018
d3eaab9
write _notAIX as non-negative constant and use 'not _AIX'
aixtools Aug 22, 2018
3469a01
added additional 'skip' entries for commands that do not exist on AIX
aixtools Aug 22, 2018
399e8ec
Use and set a single _NODE_GETTERS constant with platform test outsid…
aixtools Aug 24, 2018
6e2a9bf
modify the lambda function so it does not look like a bug
aixtools Sep 16, 2018
bb3a460
resolve differences in imports
aixtools Sep 16, 2018
db6767f
Switch back to using sys.platform.startswith() to resolve conflicts
aixtools Sep 16, 2018
25d3ef1
still trying to get differencs to resolve...
aixtools Sep 16, 2018
fa9b43b
changes per request. Thx for the review!
aixtools Oct 28, 2018
4a0c8f0
resolve merge conflict
aixtools Oct 28, 2018
6463707
resolve merge conflict, 2nd try
aixtools Oct 28, 2018
a0ef760
remove unused added import
taleinat Nov 12, 2018
095e221
Per requested changes
aixtools Nov 14, 2018
ec4c0e8
Merge branch 'bpo-28009-1' of github.com:aixtools/cpython into bpo-28…
aixtools Nov 14, 2018
70a45f0
Merge branch 'master' into bpo-28009-1
ncoghlan Dec 26, 2018
b1b4952
sync with master
aixtools Jun 14, 2019
8f0687a
rename find_mac_routines (in test_uuid.py)
aixtools Jun 14, 2019
4756670
fix sync issues
aixtools Jun 14, 2019
c55714a
sync with master
aixtools Jun 16, 2019
10f272e
add inline comments, correct typos
aixtools Jun 17, 2019
27b6c32
Improve blurb
aixtools Jun 21, 2019
33969b9
refactor more of getting a command's stdout into the helper func
taleinat Jun 24, 2019
7a57734
reduce scope of try/except blocks, for clarity
taleinat Jun 24, 2019
4628cea
make tests run regardless of host OS
taleinat Jun 24, 2019
d2830a2
remove no longer necessary _AIX definition in the test file
taleinat Jul 14, 2019
0688727
fix unittest import and indentation in one place
taleinat Jul 14, 2019
ff1ee20
simplify macaddr parsing and control flow in _find_mac_nextlines()
taleinat Jul 14, 2019
083e9c6
no need for extra return value check in _netstart_getnode()
taleinat Jul 14, 2019
30cd017
improve wording of NEWS entry
taleinat Jul 14, 2019
27a972b
make AIX formatted MAC address handling AIX-specific
taleinat Jul 15, 2019
543e66d
Stop reverse DNS lookups with netstat
aixtools Jul 15, 2019
588fda7
add _MAC_OMITS_LEADING_ZEROES, better function names and doc-strings
taleinat Jul 17, 2019
28f7a01
update blurb and comments in test_uuid.py
aixtools Aug 30, 2019
6fc2129
Update News blurb
aixtools Sep 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 45 additions & 16 deletions Lib/test/test_uuid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@
import shutil
import subprocess
import sys
from unittest import mock

py_uuid = support.import_fresh_module('uuid', blocked=['_uuid'])
c_uuid = support.import_fresh_module('uuid', fresh=['_uuid'])

_AIX = sys.platform.startswith("aix")

def importable(name):
try:
Expand Down Expand Up @@ -461,8 +460,7 @@ def test_uuid1_eui64(self):
with unittest.mock.patch.multiple(
self.uuid,
_node=None, # Ignore any cached node value.
_NODE_GETTERS_WIN32=[too_large_getter],
_NODE_GETTERS_UNIX=[too_large_getter],
_NODE_GETTERS=[too_large_getter],
):
node = self.uuid.getnode()
self.assertTrue(0 < node < (1 << 48), '%012x' % node)
Expand Down Expand Up @@ -569,19 +567,19 @@ def test_uuid1_bogus_return_value(self):
self.assertEqual(u.is_safe, self.uuid.SafeUUID.unknown)

def test_uuid1_time(self):
with mock.patch.object(self.uuid, '_has_uuid_generate_time_safe', False), \
mock.patch.object(self.uuid, '_generate_time_safe', None), \
mock.patch.object(self.uuid, '_last_timestamp', None), \
mock.patch.object(self.uuid, 'getnode', return_value=93328246233727), \
mock.patch('time.time_ns', return_value=1545052026752910643), \
mock.patch('random.getrandbits', return_value=5317): # guaranteed to be random
with unittest.mock.patch.object(self.uuid, '_has_uuid_generate_time_safe', False), \
unittest.mock.patch.object(self.uuid, '_generate_time_safe', None), \
unittest.mock.patch.object(self.uuid, '_last_timestamp', None), \
unittest.mock.patch.object(self.uuid, 'getnode', return_value=93328246233727), \
unittest.mock.patch('time.time_ns', return_value=1545052026752910643), \
unittest.mock.patch('random.getrandbits', return_value=5317): # guaranteed to be random
u = self.uuid.uuid1()
self.assertEqual(u, self.uuid.UUID('a7a55b92-01fc-11e9-94c5-54e1acf6da7f'))

with mock.patch.object(self.uuid, '_has_uuid_generate_time_safe', False), \
mock.patch.object(self.uuid, '_generate_time_safe', None), \
mock.patch.object(self.uuid, '_last_timestamp', None), \
mock.patch('time.time_ns', return_value=1545052026752910643):
with unittest.mock.patch.object(self.uuid, '_has_uuid_generate_time_safe', False), \
unittest.mock.patch.object(self.uuid, '_generate_time_safe', None), \
unittest.mock.patch.object(self.uuid, '_last_timestamp', None), \
unittest.mock.patch('time.time_ns', return_value=1545052026752910643):
u = self.uuid.uuid1(node=93328246233727, clock_seq=5317)
self.assertEqual(u, self.uuid.UUID('a7a55b92-01fc-11e9-94c5-54e1acf6da7f'))

Expand Down Expand Up @@ -669,14 +667,41 @@ class TestUUIDWithExtModule(BaseTestUUID, unittest.TestCase):
class BaseTestInternals:
uuid = None

@unittest.skipUnless(_AIX, 'requires AIX')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the skip logic changes that you extracted out to other PRs, a better check here will be @unittest.skipUnless(self.uuid._MAC_DELIM == b'.'), since that's what the sample data is assuming.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also suggests abstracting out a marker attribute for uuid._MAC_OMITS_LEADING_ZEROES, so the full check would be:

@unittest.skipUnless(self.uuid._MAC_OMITS_LEADINGS_ZEROES)
@unittest.skipUnless(self.uuid._MAC_DELIM == b'.')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You (or I) resolved this. Both marker attributes are defined, and rather than skip, are assigned to the mock tests.

If I have missed something - I'll get on it, if not - please consider approving the changes, Thx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that this is a valid test anywhere that those two conditions are true, so rather than testing for "AIX_" here, you can just test for the MAC notation conventions that the test cares about.

def test_find_mac_netstat(self):
data = '''\
Name Mtu Network Address Ipkts Ierrs Opkts Oerrs Coll
en0 1500 link#2 fe.ad.c.1.23.4 1714807956 0 711348489 0 0
01:00:5e:00:00:01
en0 1500 192.168.129 x071 1714807956 0 711348489 0 0
224.0.0.1
en0 1500 192.168.90 x071 1714807956 0 711348489 0 0
224.0.0.1
'''
popen = unittest.mock.MagicMock()
popen.stdout = io.BytesIO(data.encode())

with unittest.mock.patch.object(shutil, 'which',
return_value='/usr/bin/netstat'):
with unittest.mock.patch.object(subprocess, 'Popen',
return_value=popen):
mac = self.uuid._find_mac_netstat(
command='netstat',
args='-ia',
hw_identifiers=b'Address',
get_index=lambda x: x,
)

self.assertEqual(mac, 0xfead0c012304)

@unittest.skipUnless(os.name == 'posix', 'requires Posix')
@unittest.skipIf(_AIX, 'AIX "ifconfig" does not provide macaddr')
def test_find_mac(self):
data = '''
fake hwaddr
fake Link encap:UNSPEC hwaddr 00-00
cscotun0 Link encap:UNSPEC HWaddr 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
eth0 Link encap:Ethernet HWaddr 12:34:56:78:90:ab
'''

popen = unittest.mock.MagicMock()
popen.stdout = io.BytesIO(data.encode())

Expand All @@ -703,21 +728,25 @@ def check_node(self, node, requires=None):
"%s is not an RFC 4122 node ID" % hex)

@unittest.skipUnless(os.name == 'posix', 'requires Posix')
@unittest.skipIf(_AIX, 'AIX "ifconfig" does not provide macaddr')
def test_ifconfig_getnode(self):
node = self.uuid._ifconfig_getnode()
self.check_node(node, 'ifconfig')

@unittest.skipUnless(os.name == 'posix', 'requires Posix')
@unittest.skipIf(_AIX, 'requires command "ip"')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unittest.skipUnless(uuid._ip_getnode in uuid._NODE_GETTERS, "ip is not used for introspection on this platform")

def test_ip_getnode(self):
node = self.uuid._ip_getnode()
self.check_node(node, 'ip')

@unittest.skipUnless(os.name == 'posix', 'requires Posix')
@unittest.skipIf(_AIX, 'AIX "arp" does not provide macaddr')
def test_arp_getnode(self):
node = self.uuid._arp_getnode()
self.check_node(node, 'arp')

@unittest.skipUnless(os.name == 'posix', 'requires Posix')
@unittest.skipIf(_AIX, 'requires command "lanscan"')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unittest.skipUnless(uuid._lanscan_getnode in uuid._NODE_GETTERS, "lanscan is not used for introspection on this platform")

def test_lanscan_getnode(self):
node = self.uuid._lanscan_getnode()
self.check_node(node, 'lanscan')
Expand Down
97 changes: 58 additions & 39 deletions Lib/uuid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@

__author__ = 'Ka-Ping Yee <[email protected]>'

_WIN32 = sys.platform == 'win32'
_AIX = sys.platform.startswith("aix")
_MAC_DELIM = b':' if not _AIX else b'.'

RESERVED_NCS, RFC_4122, RESERVED_MICROSOFT, RESERVED_FUTURE = [
'reserved for NCS compatibility', 'specified in RFC 4122',
'reserved for Microsoft compatibility', 'reserved for future definition']
Expand Down Expand Up @@ -390,7 +394,7 @@ def _find_mac(command, args, hw_identifiers, get_index):
if words[i] in hw_identifiers:
try:
word = words[get_index(i)]
mac = int(word.replace(b':', b''), 16)
mac = int(word.replace(_MAC_DELIM, b''), 16)
if _is_universal(mac):
return mac
first_local_mac = first_local_mac or mac
Expand All @@ -405,6 +409,48 @@ def _find_mac(command, args, hw_identifiers, get_index):
pass
return first_local_mac or None

def _find_mac_netstat(command, args, hw_identifiers, get_index):
first_local_mac = None
mac = None
try:
proc = _popen(command, *args.split())
if not proc:
return None
with proc:
words = proc.stdout.readline().rstrip().split()
try:
i = words.index(hw_identifiers)
except ValueError:
return None
for line in proc.stdout:
try:
words = line.rstrip().split()
word = words[get_index(i)]
if len(word) == 17 and word.count(_MAC_DELIM) == 5:
mac = int(word.replace(_MAC_DELIM, b''), 16)
elif _AIX and word.count(_MAC_DELIM) == 5:
# the extracted hex string is not a 12 hex digit
# string, so add the fields piece by piece
if len(word) < 17 and len(word) >= 11:
mac = 0
fields = word.split(_MAC_DELIM)
for hex in fields:
mac <<= 8
mac += int(hex, 16)
if mac and _is_universal(mac):
return mac
first_local_mac = first_local_mac or mac
except (ValueError, IndexError):
# Virtual interfaces, such as those provided by
# VPNs, do not have a colon-delimited MAC address
# as expected, but a 16-byte HWAddr separated by
# dashes. These should be ignored in favor of a
# real MAC address
pass
except OSError:
pass
return first_local_mac or None

def _ifconfig_getnode():
"""Get the hardware address on Unix by running ifconfig."""
# This works on Linux ('' or '-a'), Tru64 ('-av'), but not all Unixes.
Expand Down Expand Up @@ -456,32 +502,9 @@ def _lanscan_getnode():

def _netstat_getnode():
"""Get the hardware address on Unix by running netstat."""
# This might work on AIX, Tru64 UNIX.
first_local_mac = None
try:
proc = _popen('netstat', '-ia')
if not proc:
return None
with proc:
words = proc.stdout.readline().rstrip().split()
try:
i = words.index(b'Address')
except ValueError:
return None
for line in proc.stdout:
try:
words = line.rstrip().split()
word = words[i]
if len(word) == 17 and word.count(b':') == 5:
mac = int(word.replace(b':', b''), 16)
if _is_universal(mac):
return mac
first_local_mac = first_local_mac or mac
except (ValueError, IndexError):
pass
except OSError:
pass
return first_local_mac or None
# This works on AIX and might work on Tru64 UNIX.
mac = _find_mac_netstat('netstat', '-ia', b'Address', lambda i: i)
return mac or None

def _ipconfig_getnode():
"""Get the hardware address on Windows by running ipconfig.exe."""
Expand Down Expand Up @@ -673,13 +696,15 @@ def _random_getnode():
return random.getrandbits(48) | (1 << 40)


if _AIX:
_NODE_GETTERS = [_unix_getnode, _netstat_getnode]
elif _WIN32:
_NODE_GETTERS = [_windll_getnode, _netbios_getnode, _ipconfig_getnode]
else:
_NODE_GETTERS = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]
_node = None

_NODE_GETTERS_WIN32 = [_windll_getnode, _netbios_getnode, _ipconfig_getnode]

_NODE_GETTERS_UNIX = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]

def getnode(*, getters=None):
"""Get the hardware address as a 48-bit positive integer.

Expand All @@ -692,12 +717,7 @@ def getnode(*, getters=None):
if _node is not None:
return _node

if sys.platform == 'win32':
getters = _NODE_GETTERS_WIN32
else:
getters = _NODE_GETTERS_UNIX

for getter in getters + [_random_getnode]:
for getter in _NODE_GETTERS + [_random_getnode]:
try:
_node = getter()
except:
Expand All @@ -706,7 +726,6 @@ def getnode(*, getters=None):
return _node
assert False, '_random_getnode() returned invalid value: {}'.format(_node)


_last_timestamp = None

def uuid1(node=None, clock_seq=None):
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2018-08-04-12-26-11.bpo-28009.4JcHZb.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix uuid.uuid1() and uuid.get_node() on AIX