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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

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

wants to merge 8 commits into from

Conversation

aixtools
Copy link
Contributor

@aixtools aixtools commented Jan 14, 2018 •
edited by bedevere-bot
Loading

  • Add uuid._generate_time_safe using ctypes() from libc.uuid_create (for when _uuid is not available)
  • use the character '.' rather than ':' to separate the MAC ADDR values
    returned by 'netstat -i' command (AIX specific)
  • The commands arp and ifconfig do not return a local MAC address

https://bugs.python.org/issue28009

@aixtools
* in Lib/uuid.py define ctypes() based definition of
7837745 
  util._generate_time_safe based on lib.uuid_create (for when _uuid is not available)
* use the character '.' rather than ':' to seperate the MAC ADDR values
  returned by 'netstat -i' command (AIX specific)
* The commands arp and ifconfig do not return a local MAC address
@aixtools
correct typo in News
f6d95f3
@aixtools
Copy link
Contributor Author

aixtools commented Feb 5, 2018

I wanted this in:
michael@x071:[/data/prj/python/git/python3-3.7]diff -u Lib/uuid.py ../uuid.py-3.8
--- Lib/uuid.py 2018-02-05 16:07:49 +0000
+++ ../uuid.py-3.8 2018-02-05 15:21:09 +0000
@@ -708,6 +708,8 @@
_NODE_GETTERS_UNIX = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]

+_NODE_GETTERS_AIX = [_unix_getnode, _netstat_getnode]
+
def getnode(*, getters=None):
"""Get the hardware address as a 48-bit positive integer.

@@ -722,6 +724,8 @@

 if sys.platform == 'win32':
     getters = _NODE_GETTERS_WIN32
  • elif sys.platform.startswith("aix"):
  •    getters = _NODE_GETTERS_AIX
    else:
    getters = _NODE_GETTERS_UNIX

but continuously creates a merge conflict. C'est la vie.

native-api
native-api suggested changes May 17, 2018
View reviewed changes
Copy link
Contributor

@native-api native-api left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As a general note, AIX is a niche system and should have the absolute possible minimum of code specific to it -- if anything, because the devteam can't test and thus maintain the correctness of that code. For the same reason, it should disrupt control flow the least amount possible -- otherwise, those untestable chunks will put limitations on further changes. Whenever possible, UNIX flavors should be distinguished by functional differences rather than names.

Lib/uuid.py
if sys.platform.startswith("aix"):
c_field = b'.'
else:
c_field = b':'
Copy link
Contributor

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)

Lib/uuid.py
return int(word.replace(b'.', b''), 16)
else:
return None

Copy link
Contributor

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
Contributor

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?

Lib/uuid.py
@@ -573,12 +598,18 @@ 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_create() is used by other POSIX platforms (e.g., AIX, FreeBSD)
Copy link
Contributor

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

Lib/uuid.py
# lib value, then look for uuid_generate()
if not lib:
try:
lib = ctypes.CDLL(None)
Copy link
Contributor

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.

Lib/uuid.py
_buffer = ctypes.create_string_buffer(16)# uuid
_status = ctypes.create_string_buffer(2) # c_ushort
_uuid_generate_time(_buffer, _status)
return bytes(_buffer.raw), bytes(_status.raw)
Copy link
Contributor

Choose a reason for hiding this comment

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

This only searches the last found lib, whatever it happens to be.

Better search uuid_create with other exports and save in a temporary var if found. After the loop, execute this code if uuid_create is found and others aren't.

native-api
native-api suggested changes May 17, 2018
View reviewed changes
Lib/uuid.py
_NODE_GETTERS_UNIX = [_unix_getnode, _ifconfig_getnode, _ip_getnode,
_arp_getnode, _lanscan_getnode, _netstat_getnode]

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

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.

Lib/uuid.py
return _node
assert False, '_random_getnode() returned None'
assert False, '_random_getnode() returned invalid value: {}'.format(_node)
Copy link
Contributor

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.

vstinner
vstinner reviewed Jun 4, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

"define uuid._generate_time_safe in uuid.py based on ctypes() lib.uuid_create (for when _uuid is not available)"

When is _uuid not available? Why not fixing _uuid instead, if it has issues?

@vstinner vstinner requested a review from pitrou June 4, 2018 09:00
@aixtools
Copy link
Contributor Author

aixtools commented Jun 4, 2018 via email

@vstinner
Copy link
Member

vstinner commented Jun 4, 2018

_uuid is fixed in AIX 6.1 and later, but not in AIX 5.3 as AIX 5.3 does not have uuid_create() in libc.

What is available on AIX 5.3?

@aixtools
Copy link
Contributor Author

aixtools commented Jun 4, 2018 via email

@vstinner
Copy link
Member

vstinner commented Jun 4, 2018

I don't understand why ctypes is needed. You wrote that AIX 6.1 can use _uuid, but AIX 5.3 has no usable system uuid library. If AIX doesn't benefit of ctypes, would you mind to simplify your PR to only fix the AIX specific issue (the colon character issue)?

@aixtools
Copy link
Contributor Author

aixtools commented Jun 5, 2018 via email

@vstinner
Copy link
Member

vstinner commented Jun 5, 2018

"Yes, I can do that. However, I still hope you will consider backporting this - as it has been broken everywhere for a long time."

I don't understand well the AIX platform. This PR is very long and so hard to review. I suggested you to write a shorter PR so it should be simpler to get a review and fix the master branch (and 3.7 which also has _uuid).

Once master (and 3.7) will be fixed, we can start discussing how to fix other branches.

I don't like the idea of adding code to the master branch which is written for older branches. I prefer to not "add legacy code" to the master branch. What do you think of my rationale?

@aixtools
Copy link
Contributor Author

aixtools commented Jun 5, 2018 via email

@vstinner
Copy link
Member

vstinner commented Jun 5, 2018

You can reuse bpo-28009 to fix _uuid in master/3.7, but please create a different PR.

@aixtools
Copy link
Contributor Author

aixtools commented Jun 6, 2018 via email

@vstinner
Copy link
Member

vstinner commented Jun 6, 2018

The development starts in master, once it's stabilized, it's merged into 3.7, and then to 3.6 and maybe 2.7. I'm talking about the general case. I don't know until which version we should backport these enhancements/fixes.

@aixtools
Copy link
Contributor Author

aixtools commented Jun 6, 2018 via email

@aixtools
Copy link
Contributor Author

Will start a new PR now that _uuid is working in master

@aixtools aixtools closed this Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@native-api native-api native-api requested changes

@pitrou pitrou Awaiting requested review from pitrou

Assignees
No one assigned
Labels
awaiting core review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aixtools @vstinner @native-api @the-knights-who-say-ni @bedevere-bot