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
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

aixtools wants to merge 8 commits into from

Conversation

@aixtools
Copy link
Contributor

@aixtoolsaixtools 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
ContributorAuthor

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-apinative-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
ifsys.platform.startswith("aix"):
c_field=b'.'
else:
c_field=b':'
Copy link
Contributor

@native-apinative-apiMay 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)

Lib/uuid.py
returnint(word.replace(b'.', b''), 16)
else:
returnNone

Copy link
Contributor

@native-apinative-apiMay 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
Contributor

@native-apinative-apiMay 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?

Lib/uuid.py

# 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

@native-apinative-apiMay 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

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

@native-apinative-apiMay 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.

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

@native-apinative-apiMay 17, 2018

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]

defgetnode(*, getters=None):
Copy link
Contributor

@native-apinative-apiMay 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.

Lib/uuid.py
if(_nodeisnotNone) and (0<=_node< (1<<48)):
return_node
assertFalse, '_random_getnode() returned None'
assertFalse, '_random_getnode() returned invalid value:{}'.format(_node)
Copy link
Contributor

@native-apinative-apiMay 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.

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

@vstinnervstinner 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?

@vstinnervstinner requested a review from pitrouJune 4, 2018 09:00
@aixtools
Copy link
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

aixtools commented Jun 6, 2018 via email

@aixtools
Copy link
ContributorAuthor

aixtools commented Jun 10, 2018

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments

@pitroupitrouAwaiting requested review from pitrou

+1 more reviewer

@native-apinative-apinative-api requested changes

Reviewers whose approvals may not affect merge requirements

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