Skip to content

13914 use read/write locking on Windows#14381

Closed
VeeFu wants to merge 1 commit intoopenssl:masterfrom
VeeFu:Fix-13914-use-slim-read-write-on-windows
Closed

13914 use read/write locking on Windows#14381
VeeFu wants to merge 1 commit intoopenssl:masterfrom
VeeFu:Fix-13914-use-slim-read-write-on-windows

Conversation

@VeeFu
Copy link
Copy Markdown
Contributor

@VeeFu VeeFu commented Mar 1, 2021 •
edited
Loading 

Use read/write locking on Windows

Fixes #13914

The "SRWLock" synchronization primitive is available in Windows Vista
and later.  CRYPTO_THREAD functions now use SRWLock functions when the
target operating system supports them.
Checklist

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Mar 1, 2021
@VeeFu VeeFu mentioned this pull request Mar 2, 2021
Closed
@VeeFu VeeFu marked this pull request as draft March 2, 2021 04:19
slontis
slontis reviewed Mar 4, 2021
View reviewed changes
slontis
slontis reviewed Mar 4, 2021
View reviewed changes
slontis
slontis reviewed Mar 4, 2021
View reviewed changes
@slontis
Copy link
Copy Markdown
Member

slontis commented Mar 4, 2021

Should this have a CHANGES entry?

@paulidale paulidale added the branch: master Applies to master branch label Mar 4, 2021
paulidale
paulidale requested changes Mar 4, 2021
View reviewed changes
slontis
slontis reviewed Mar 4, 2021
View reviewed changes
@slontis
Copy link
Copy Markdown
Member

slontis commented Mar 4, 2021

Once #14319 is merged (in a day or so), you should be able to rebase this and remove the Merge branch..

Rewording the commit message of the first commit to just say...

Use read/write locking on Windows

Fixes #13914

The "SRWLock" ....

mattcaswell
mattcaswell reviewed Mar 4, 2021
View reviewed changes
@mattcaswell
Copy link
Copy Markdown
Member

mattcaswell commented Mar 4, 2021

Should this have a CHANGES entry?

Yes - I think a CHANGES.md entry is warranted for this.

@VeeFu
Copy link
Copy Markdown
Contributor Author

VeeFu commented Mar 4, 2021

Once #14319 is merged (in a day or so), you should be able to rebase this and remove the Merge branch..

Yes... the merge commit was a failed attempt to replace the first commit, which was misattributed to my work email address. I'm not quite sure how to fix this. Re-pushing the remote branch comes to mind, but I thought that might lose review history. I do intend to clean this up before merge; I'll need to learn a bit more git before I can, though.

mattcaswell
mattcaswell requested changes Mar 4, 2021
View reviewed changes
@t8m
Copy link
Copy Markdown
Member

t8m commented Mar 4, 2021

Once #14319 is merged (in a day or so), you should be able to rebase this and remove the Merge branch..

Yes... the merge commit was a failed attempt to replace the first commit, which was misattributed to my work email address. I'm not quite sure how to fix this. Re-pushing the remote branch comes to mind, but I thought that might lose review history. I do intend to clean this up before merge; I'll need to learn a bit more git before I can, though.

Do not worry. Just force push with clean commits.

@mattcaswell
Copy link
Copy Markdown
Member

mattcaswell commented Mar 8, 2021

#14319 just got merged...

@VeeFu VeeFu requested review from mattcaswell and paulidale March 8, 2021 15:29
@VeeFu
Copy link
Copy Markdown
Contributor Author

VeeFu commented Mar 8, 2021

#14319 just got merged...

Thanks, I'll rebase, have another go at deleting the first commit (causing the CLA violation) and force-push the branch.

mattcaswell
mattcaswell approved these changes Mar 8, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Approved subject to:

  • the nit mentioned below being fixed, and
  • the CIs passing once this is rebased and the merge commit removed.

crypto/threads_win.c Outdated
Copy link
Copy Markdown
Member

@mattcaswell mattcaswell Mar 8, 2021

Choose a reason for hiding this comment

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

Put the else on the same line as }

} else {

Copy link
Copy Markdown
Contributor Author

@VeeFu VeeFu Mar 8, 2021

Choose a reason for hiding this comment

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

nit picked.

@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Mar 8, 2021
@mattcaswell
Copy link
Copy Markdown
Member

mattcaswell commented Mar 8, 2021

I guess this PR can be taken out of "draft" now.

@VeeFu VeeFu force-pushed the Fix-13914-use-slim-read-write-on-windows branch from 07543ad to ca9564f Compare March 8, 2021 17:04
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Mar 8, 2021
@VeeFu VeeFu force-pushed the Fix-13914-use-slim-read-write-on-windows branch from ca9564f to 9211cf6 Compare March 8, 2021 17:08
@VeeFu VeeFu marked this pull request as ready for review March 8, 2021 17:09
mattcaswell
mattcaswell approved these changes Mar 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Reconfirming my approval. The CI failures all appear to be known issues unrelated to this PR.

Ping for second review.

@slontis
Copy link
Copy Markdown
Member

slontis commented Mar 9, 2021

Needs to be rebased..

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 9, 2021
@paulidale
Copy link
Copy Markdown
Contributor

paulidale commented Mar 9, 2021

It needs a rebase but merging the changes entry shouldn't impact approval.

@VeeFu VeeFu force-pushed the Fix-13914-use-slim-read-write-on-windows branch from 9211cf6 to 4230852 Compare March 10, 2021 14:53
@VeeFu
Use read/write locking on Windows
f229aa9 
Fixes openssl#13914

The "SRWLock" synchronization primitive is available in Windows Vista
and later.  CRYPTO_THREAD functions now use SRWLock functions when the
target operating system supports them.
@VeeFu VeeFu force-pushed the Fix-13914-use-slim-read-write-on-windows branch from 4230852 to f229aa9 Compare March 10, 2021 20:47
@openssl-machine
Copy link
Copy Markdown
Collaborator

openssl-machine commented Mar 10, 2021

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 11, 2021
@mattcaswell
Copy link
Copy Markdown
Member

mattcaswell commented Mar 11, 2021

Pushed! Thank you for your contribution!!

openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
@VeeFu @mattcaswell
Use read/write locking on Windows
f70863d 
Fixes #13914

The "SRWLock" synchronization primitive is available in Windows Vista
and later.  CRYPTO_THREAD functions now use SRWLock functions when the
target operating system supports them.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #14381)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slontis slontis slontis left review comments

@mattcaswell mattcaswell mattcaswell approved these changes

@paulidale paulidale paulidale approved these changes

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@VeeFu @slontis @mattcaswell @t8m @paulidale @openssl-machine