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

Fix sctp compile errors #19398

Closed
wants to merge 4 commits into from
Closed

Fix sctp compile errors #19398

wants to merge 4 commits into from

Conversation

slontis
Copy link
Member

@slontis slontis commented Oct 12, 2022 •
edited

Fixes #19371

running config with 'enable-sctp' gave compiler errors.

@slontis
Copy link
Member Author

slontis commented Oct 12, 2022 •
edited

Tests passed locally with these changes..

To add this to our ci loop it would need to do the following:

sudo apt install libsctp-dev
sysctl -w net.sctp.auth_enable=1

@slontis slontis added branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch and removed branch: 3.0 Merge to openssl-3.0 branch labels Oct 12, 2022
@t8m t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug labels Oct 12, 2022
t8m
t8m previously approved these changes Oct 12, 2022
View reviewed changes
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Oct 12, 2022
paulidale
paulidale requested changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment •
edited

Choose a reason for hiding this comment

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

The fix here looks good but I would expect a enable-sctp build to be added to the CIs. Possibly in parallel with or replacing the runchecker no-sctp build.

sudo works in CIs. It might need a conditional build step so this only gets added for the enable-sctp build.
If added to one of the non-matrix builds, the install/setup sets would be easy to add.

@slontis
Copy link
Member Author

slontis commented Oct 12, 2022

Did you see the preconditions to doing this pauli?

@paulidale
Copy link
Contributor

paulidale commented Oct 12, 2022 •
edited

Yes, they should be okay in an actions build. We apt-get install things in several builds already.

I was editing my note at the time you commented.

@slontis
Copy link
Member Author

slontis commented Oct 13, 2022 •
edited

@paulidale do I have to do trickery do test the actions file

mattcaswell
mattcaswell requested changes Oct 13, 2022
View reviewed changes
ssl/record/rec_layer_d1.c Outdated Show resolved Hide resolved
ssl/record/rec_layer_d1.c Outdated Show resolved Hide resolved
ssl/record/rec_layer_d1.c Outdated Show resolved Hide resolved
@bernd-edlinger
Copy link
Member

Isn't the commit message mis-typed?
"sctp" instead of "stcp"

@slontis slontis changed the title Fix stcp compile errors Fix sctp compile errors Oct 13, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Oct 13, 2022
@slontis slontis force-pushed the stcp_fix branch 2 times, most recently from 52127a1 to 3fc6df4 Compare October 14, 2022 01:43
@slontis
Copy link
Member Author

slontis commented Oct 14, 2022

Testing with a dummy action show that 'lksctp-tools' needed to be installed also..
The tests passed with this setup. The tests also failed if net.sctp.auth_enable was not set (indicating that the sctp is being used in the tests).

paulidale
paulidale approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

With the if line moved up.

.github/workflows/run-checker-daily.yml
sudo apt-get update
sudo apt-get -yq install lksctp-tools libsctp-dev
sudo sysctl -w net.sctp.auth_enable=1
if: matrix.opt == 'enable-sctp'
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest putting this immediate under the name: line.

@paulidale
Copy link
Contributor

@t8m reapprove?

@t8m t8m 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 Oct 17, 2022
@openssl-machine openssl-machine 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 Oct 18, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@hlandau
Copy link
Member

hlandau commented Oct 18, 2022

Merged to master. Thank you.

@hlandau hlandau closed this Oct 18, 2022
openssl-machine pushed a commit that referenced this pull request Oct 18, 2022
@slontis @hlandau
Fix sctp compile errors
846975f 
Fixes #19371

running config with 'enable-sctp' gave compiler errors.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #19398)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@slontis @beldmit
Fix sctp compile errors
d253425 
Fixes openssl#19371

running config with 'enable-sctp' gave compiler errors.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#19398)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@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 Merge to master branch severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Master branch build is broken with enable-sctp option
7 participants
@slontis @paulidale @bernd-edlinger @openssl-machine @hlandau @t8m @mattcaswell