Skip to content

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

Merged
vstinner merged 1 commit into
python:masterfrom
serhiy-storchaka:uuid-create-freebsd2
May 24, 2018
Merged

bpo-32493: Fix uuid.uuid1() on FreeBSD.#7099
vstinner merged 1 commit into
python:masterfrom
serhiy-storchaka:uuid-create-freebsd2

Conversation

@serhiy-storchaka

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

Copy link
Copy Markdown
Member

An alternate solution using uuid_enc_be().

https://bugs.python.org/issue32493

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

ned-deily commented May 24, 2018

Copy link
Copy Markdown
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

ned-deily commented May 24, 2018

Copy link
Copy Markdown
Member

@vstinner, any opinion?

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

@vstinnervstinnerMay 24, 2018

Copy link
Copy Markdown
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"

@vstinnervstinnerMay 24, 2018

Copy link
Copy Markdown
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.

@vstinnervstinnerMay 24, 2018

Copy link
Copy Markdown
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

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@vstinner
vstinnerforce-pushed the uuid-create-freebsd2 branch from fbfe6bf to fd869ceCompareMay 24, 2018 21:39
@vstinner

vstinner commented May 24, 2018

Copy link
Copy Markdown
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

vstinner commented May 24, 2018

Copy link
Copy Markdown
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:masterMay 24, 2018
@miss-islington

miss-islington commented May 24, 2018

Copy link
Copy Markdown
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

bedevere-bot commented May 24, 2018

Copy link
Copy Markdown

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>
@aixtoolsaixtools mentioned this pull request Jun 6, 2018
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner approved these changes

Assignees

No one assigned

Labels

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