Skip to content

bpo-28009: for AIX correct uuid.getnode() and add ctypes() based uuid._generate_time_safe #5183

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

Closed
aixtools wants to merge 8 commits into from
Closed

bpo-28009: for AIX correct uuid.getnode() and add ctypes() based uuid._generate_time_safe #5183

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7837745
* in Lib/uuid.py define ctypes() based definition of
aixtools Jan 14, 2018
f6d95f3
correct typo in News
aixtools Jan 14, 2018
44aa616
fix whitespace
aixtools Jan 14, 2018
64228cd
Ensure that 'lib' is defined before it is used
aixtools Jan 15, 2018
9d8148e
resolve conflicts in uuid.py
aixtools Feb 5, 2018
e6d1c0e
resolv conflict in uuid.py
aixtools Feb 5, 2018
dd5f39a
merge with new uuid.py from master
aixtools Feb 5, 2018
7b25dd3
uuid.py does not merge my additions, will try without the added effic…
aixtools Feb 5, 2018
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
Prev Previous commit
Next Next commit
merge with new uuid.py from master
  • Loading branch information
@aixtools
aixtools committed Feb 5, 2018
commit dd5f39a19955dc8044e92199e50a44eed0709b7b
46 changes: 18 additions & 28 deletions Lib/uuid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,9 @@ def _is_universal(mac):
def _find_mac(command, args, hw_identifiers, get_index):
# issue28009: AIX uses character '.' rather than ':'
if sys.platform.startswith("aix"):
old = b'.'
c_field = b'.'
else:
old = b':'
c_field = b':'
Copy link
Copy Markdown
Contributor

@native-api native-api May 17, 2018

Choose a reason for hiding this comment

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

  • Comments should only provide required info that's not apparent from the code/repo metadata. In particular, issue ref is only useful if the matter isn't resolved yet. Should be like: AIX <commands involved> use '.' as MAC delimiter
  • ternary form to reduce size & duplication and a more descriptive var name (e.g. mac_delim)

first_local_mac = None
try:
proc = _popen(command, *args.split())
Expand All @@ -378,7 +378,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(old, b''), 16)
mac = int(word.replace(c_field, b''), 16)
if _is_universal(mac):
return mac
first_local_mac = first_local_mac or mac
Expand All @@ -396,8 +396,6 @@ def _find_mac(command, args, hw_identifiers, get_index):
def _ifconfig_getnode():
"""Get the hardware address on Unix by running ifconfig."""
# This works on Linux ('' or '-a'), Tru64 ('-av'), but not all Unixes.
if sys.platform.startswith("aix"):
return None
keywords = (b'hwaddr', b'ether', b'address:', b'lladdr')
for args in ('', '-a', '-av'):
mac = _find_mac('ifconfig', args, keywords, lambda i: i+1)
Expand All @@ -408,17 +406,13 @@ def _ifconfig_getnode():
def _ip_getnode():
"""Get the hardware address on Unix by running ip."""
# This works on Linux with iproute2.
if sys.platform.startswith("aix"):
return None
mac = _find_mac('ip', 'link', [b'link/ether'], lambda i: i+1)
if mac:
return mac
return None

def _arp_getnode():
"""Get the hardware address on Unix by running arp."""
if sys.platform.startswith("aix"):
return None
import os, socket
try:
ip_addr = socket.gethostbyname(socket.gethostname())
Expand All @@ -445,8 +439,6 @@ def _arp_getnode():

def _lanscan_getnode():
"""Get the hardware address on Unix by running lanscan."""
if sys.platform.startswith("aix"):
return None
# This might work on HP-UX.
return _find_mac('lanscan', '-ai', [b'lan0'], lambda i: 0)

Expand All @@ -455,13 +447,17 @@ def _netstat_getmac_posix(words,i):
word = words[i]
if len(word) == 17 and word.count(b':') == 5:
return(int(word.replace(b':', b''), 16))
else:
return None

def _netstat_getmac_aix(words,i):
"""Extract the MAC address from netstat -ia specific for AIX netstat."""
word = words[i]
wlen = len(word)
if wlen >= 11 and wlen <=17 and word.count(b'.') == 5:
return int(word.replace(b'.', b''), 16)
else:
return None

Copy link
Copy Markdown
Contributor

@native-api native-api May 17, 2018

Choose a reason for hiding this comment

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

  • A great deal of duplication between the two routines. Can be consolidated. Heed the general CR comment.
  • Include a comment with samples of the text chunk being parsed in both cases so that the magic numbers make sense.
  • Reuse the above MAC delimiter var -- maybe as a global var?

Copy link
Copy Markdown
Contributor

@native-api native-api May 17, 2018

Choose a reason for hiding this comment

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

In the light of "functional differences better than names" -- probably detect the format instead?

def _netstat_getnode():
"""Get the hardware address on Unix by running netstat."""
Expand All @@ -482,10 +478,10 @@ def _netstat_getnode():
words = line.rstrip().split()
if sys.platform.startswith("aix"):
mac = _netstat_getmac_aix(words,i)
if not mac:
continue
else:
mac = _netstat_getmac_posix(words,i)
if not mac:
continue
if _is_universal(mac):
return mac
first_local_mac = first_local_mac or mac
Expand Down Expand Up @@ -557,6 +553,7 @@ def _netbios_getnode():
first_local_mac = first_local_mac or mac
return first_local_mac or None


_generate_time_safe = _UuidCreate = None
_has_uuid_generate_time_safe = None

Expand Down Expand Up @@ -601,14 +598,11 @@ def _load_system_functions():

# The uuid_generate_* routines are provided by libuuid on at least
# Linux and FreeBSD, and provided by libc on Mac OS X.
# uuid_generate() is provided by libc on AIX
# (and FreeBSD issue 32493)
# uuid_create() is used by other POSIX platforms (e.g., AIX, FreeBSD)
Copy link
Copy Markdown
Contributor

@native-api native-api May 17, 2018

Choose a reason for hiding this comment

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

  • Contradicts the existing statement just before
  • fns names are formatted differently

_libnames = ['uuid']
if not sys.platform.startswith('win'):
_libnames.append('c')
# attach to a lib and look for uuid_generate* family functions
# rather than open 'None' several times
# only try to dlopen when something has been found
# also need to look for uuid_create() but only look if something was located
lib = None
for libname in _libnames:
libnm = ctypes.util.find_library(libname)
Expand Down Expand Up @@ -638,18 +632,14 @@ def _generate_time_safe():
_uuid_generate_time(_buffer)
return bytes(_buffer.raw), None
break
# when find_library("c") returns None AND _uuid is not present
# try to attach to libc using ctypes.CDLL(None)
# on AIX (at least) ctypes.CDLL(None) returns
# the equivalent of libc because libc is already dynamically linked
# to the python executable (see also #26439, #32399, #32493)
# if _uuid_generate_time has not been found (Linux) then we try libc
# as libc is already dynamically linked (or found above) verify a valid
# lib value, then look for uuid_generate()
if not lib:
try:
lib = ctypes.CDLL(None)
Copy link
Copy Markdown
Contributor

@native-api native-api May 17, 2018

Choose a reason for hiding this comment

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

How is this supposed to work? None arg effect is undocumented. According to https://www.programcreek.com/python/example/1108/ctypes.CDLL , this is going to get the main program itself (in POSIX, at least. In Windows, it raises a TypeError) which doesn't have the required export (and probably any). Looks redundant.

except:
lib = None
# look for uuid_generate() as a way to define both
# _uuid_generate_time and _generate_time_safe
if lib:
if hasattr(lib, 'uuid_create'): # pragma: nocover
_uuid_generate_time = lib.uuid_create
Expand Down Expand Up @@ -718,7 +708,7 @@ def _random_getnode():
_NODE_GETTERS_UNIX = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]

_NODE_GETTERS_AIX = [_unix_getnode, _netstat_getnode]
_NODE_GETTERS_AIX = [_unix_getnode, _netstat_getnode]

def getnode(*, getters=None):
Copy link
Copy Markdown
Contributor

@native-api native-api May 17, 2018

Choose a reason for hiding this comment

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

Can't see the purpose of any changes from here and further.

About this one specifically:

  • There's no discussion about an interface change or justification for it.
  • In any case, the getters arg is unused as the code is now.

"""Get the hardware address as a 48-bit positive integer.
Expand All @@ -744,9 +734,9 @@ def getnode(*, getters=None):
_node = getter()
except:
continue
if _node is not None:
if (_node is not None) and (0 <= _node < (1 << 48)):
return _node
assert False, '_random_getnode() returned None'
assert False, '_random_getnode() returned invalid value: {}'.format(_node)
Copy link
Copy Markdown
Contributor

@native-api native-api May 17, 2018

Choose a reason for hiding this comment

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

Getters are supposed to return valid ids or None. Any violations that warrant rejecting the value received from the OS should be checked for by the getter itself.



_last_timestamp = None
Expand Down