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.

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

Restore the ERR_FATAL_ERROR() macro #2049

Closed
wants to merge 1 commit into from
Closed

Conversation

kaduk
Copy link
Contributor

@kaduk kaduk commented Dec 8, 2016

Checklist
  • documentation is added or updated
  • CLA is signed
Description of change

Commit 0cd0a82 removed this macro
along with many unused function and reason codes; ERR_FATAL_ERROR()
was not used in the tree, but did have external consumers.

Add it back to restore the API compatibility and avoid breaking
applications for no internal benefit.

We could wrap this in an OPENSSL_API_COMPAT check if we don't want to support it indefinitely.

Commit 0cd0a82 removed this macro
along with many unused function and reason codes; ERR_FATAL_ERROR()
was not used in the tree, but did have external consumers.

Add it back to restore the API compatibility and avoid breaking
applications for no internal benefit.
@richsalz richsalz self-assigned this Dec 8, 2016
@richsalz richsalz added 1.1.0 branch: master Merge to master branch labels Dec 8, 2016
@dot-asm
Copy link
Contributor

dot-asm commented Dec 9, 2016

-1 for 1.1.0, +1 for master

@kaduk
Copy link
Contributor Author

kaduk commented Dec 9, 2016

@dot-asm could you say a bit about why? It unfortunately does not seem obvious to me.

@dot-asm
Copy link
Contributor

dot-asm commented Dec 9, 2016

could you say a bit about why [-1 for 1.1.0]?

I'm opposed to any public header modifications between letter releases. Note that this is not a veto, just an opinion.

@kaduk
Copy link
Contributor Author

kaduk commented Dec 9, 2016

Thanks for the reminder; I do remember this coming up previously, now.

@levitte
Copy link
Member

levitte commented Dec 9, 2016

We don't all agree when it comes to error codes and messages. Just sayin'

@mattcaswell
Copy link
Member

We don't all agree when it comes to error codes and messages. Just sayin'

I would go further. I am positively against a blanket ban policy on no header file changes. I do not believe that is correct - and is even a detrimental step. I see no problem with this change going into 1.1.0.

The Appveyor failure seem unrelated to this. Something to do with the UI stuff?

@mattcaswell
Copy link
Member

@richsalz will you merge?

@levitte
Copy link
Member

levitte commented Dec 12, 2016

I'll have a look at the appveyor failures later today

@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Dec 12, 2016
levitte pushed a commit that referenced this pull request Dec 12, 2016
Commit 0cd0a82 removed this macro
along with many unused function and reason codes; ERR_FATAL_ERROR()
was not used in the tree, but did have external consumers.

Add it back to restore the API compatibility and avoid breaking
applications for no internal benefit.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #2049)
levitte pushed a commit that referenced this pull request Dec 12, 2016
Commit 0cd0a82 removed this macro
along with many unused function and reason codes; ERR_FATAL_ERROR()
was not used in the tree, but did have external consumers.

Add it back to restore the API compatibility and avoid breaking
applications for no internal benefit.

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #2049)
(cherry picked from commit 036ba50)
@richsalz
Copy link
Contributor

commit 036ba50 in master and 5c75e43 in 1.1.0 Thanks!

@richsalz richsalz closed this Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants