Skip to content
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

bpo-32493: Fix uuid.uuid1() on FreeBSD. #7099

Merged
merged 1 commit into from
May 24, 2018
Merged

bpo-32493: Fix uuid.uuid1() on FreeBSD. #7099

merged 1 commit into from
May 24, 2018

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented May 24, 2018 •
edited by bedevere-bot
Loading

An alternate solution using uuid_enc_be().

https://bugs.python.org/issue32493

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.7 labels May 24, 2018
@serhiy-storchaka serhiy-storchaka mentioned this pull request May 24, 2018
Closed
@ned-deily
Copy link
Member

For what it's worth, this approach looks to me to be cleaner and simpler than that in #7098. I'm not the right person to give a thorough technical review. But perhaps we can merge this just to master initially and see what happens with buildbots there before backporting to 3.7. Unlike some of the other blocker issues for 3.7, this one is a real user-visibile bug, no?

@ned-deily
Copy link
Member

@vstinner, any opinion?

vstinner
vstinner reviewed May 24, 2018
View reviewed changes
Modules/_uuidmodule.c
uint32_t status;
uuid_create(&uuid, &status);
# if defined(HAVE_UUID_ENC_BE)
unsigned char buf[sizeof(uuid)];
Copy link
Member

Choose a reason for hiding this comment

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

The size should be 16 bytes.
https://www.freebsd.org/cgi/man.cgi?query=uuid_create
"must be large enough to hold the 16-octet binary UUID"

Copy link
Member

Choose a reason for hiding this comment

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

Since @ned-deily wants this fix, I took the liberty of fixing this issue myself.

Copy link
Member

Choose a reason for hiding this comment

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

Wait. My change only modified the size of the buffer, but not configure, not the call to Py_BuildValue two lines below. I reverted my change.

Anyway, the module constructor has an assertion for sizeof(uuid_t) == 16.

vstinner
vstinner approved these changes May 24, 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.

LGTM.

@vstinner vstinner force-pushed the uuid-create-freebsd2 branch from fbfe6bf to fd869ce Compare May 24, 2018 21:39
@vstinner
Copy link
Member

  • I added a 2nd commit to change the buf size to 16
  • Serhiy's added 3rd commit to update also configure and Py_BuildValue call
  • I saw that I forgot to configure, I decided to remove my commit using git reset --hard HEAD^ and git push --force... But I didn't notice that I removed Serhiy's 3rd commit, since I didn't know that he pushed anything

And now GitHub only shows ... nothing, all these commits and the removal of commits are hidden on this page :-(

@vstinner
Copy link
Member

@serhiy-storchaka: Sorry, it seems like we worked together on the same branch, your branch :-/ I will no longer touch this branch. I consider that the current code using sizeof(uuid_t) is fine. If you want to use 16, it's up to you.

IMHO sizeof(uuid_t) should always be 16, if it's not, we will get a safe Python exception.

I prefer to merge the current PR to quickly fix Python 3.7 for the 3.7rc1 release.

@vstinner vstinner merged commit 17d8830 into python:master May 24, 2018
@miss-islington
Copy link
Contributor

Thanks @serhiy-storchaka for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-7104 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 24, 2018
@serhiy-storchaka @miss-islington
bpo-32493: Fix uuid.uuid1() on FreeBSD. (pythonGH-7099)
4ace023 
Use uuid_enc_be() if available to encode UUID to bytes as big endian.
(cherry picked from commit 17d8830)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@serhiy-storchaka serhiy-storchaka deleted the uuid-create-freebsd2 branch May 24, 2018 22:46
miss-islington added a commit that referenced this pull request May 24, 2018
@miss-islington @serhiy-storchaka
bpo-32493: Fix uuid.uuid1() on FreeBSD. (GH-7099)
5734f41 
Use uuid_enc_be() if available to encode UUID to bytes as big endian.
(cherry picked from commit 17d8830)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@aixtools aixtools mentioned this pull request Jun 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@serhiy-storchaka @ned-deily @vstinner @miss-islington @bedevere-bot @the-knights-who-say-ni