GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111230: Fix `_ssl.c` not checking for errors when initializing a module by sobolevn · Pull Request #111232 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Oct 23, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
.. c:function:: int PyModule_AddIntConstant(PyObject *module, const char *name, long value)

   Add an integer constant to *module* as *name*.  This convenience function can be
   used from the module's initialization function. Return ``-1`` on error, ``0`` on
   success.

I've used _PyModule_ADD_INT_CONST macro name to make the diff minimal:

  • It has the same length as PyModule_AddIntConstant
  • So, you don't have to reformat the second line of code
  • Issue: _ssl.c does not handle errors on module creation #111230

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sobolevn
pythongh-111230: Fix _ssl.c not checking for errors when initializi…
45894e0
…ng a module
@sobolevn
sobolevn requested review from serhiy-storchaka and vstinner October 23, 2023 19:45
@bedevere-app bedevere-app Bot mentioned this pull request Oct 23, 2023
_ssl.c does not handle errors on module creation #111230
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Oct 23, 2023
erlend-aasland
erlend-aasland reviewed Oct 23, 2023
View reviewed changes
Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

sobolevn added 2 commits October 24, 2023 08:41
@sobolevn
Address review
81aa96a
@sobolevn
typo
0bdb97a
serhiy-storchaka
serhiy-storchaka reviewed Oct 24, 2023
View reviewed changes
Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka
serhiy-storchaka reviewed Oct 24, 2023
View reviewed changes
Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

sobolevn added 2 commits October 24, 2023 10:02
@sobolevn
Address review
042c49a
@sobolevn
typo
b47d864
erlend-aasland
erlend-aasland reviewed Oct 25, 2023
View reviewed changes
Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland reviewed Oct 25, 2023
View reviewed changes
Comment thread Modules/_ssl.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn @erlend-aasland
Apply suggestions from code review
0a3ff18
Co-authored-by: Erlend E. Aasland <[email protected]>
serhiy-storchaka
serhiy-storchaka approved these changes Oct 25, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, but I would prefer to not use ALL_CAPS for macro parameters (here, and in your other PRs). It is very unusual (and perhaps against PEP 7). Please only use ALL_CAPS for macro names.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Oct 25, 2023
@sobolevn

sobolevn commented Oct 25, 2023

Copy link
Copy Markdown
Member Author

@serhiy-storchaka I think that this is the style suggested and prefered by @erlend-aasland, I am ok with both.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Oct 25, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

It is very unusual (and perhaps against PEP 7).

There is no mention of macro arguments in PEP-7; please do not spread uncertainty and doubt like this.

I personally prefer ALL_CAPS macro parameters since it makes them stand out better from implicit variables that are not part of the macro "scope"; for example, many macros are used in extension module initialisation phase, and very often, the module object is implicitly used from within the macro. Using ALL_CAPS macro parameters makes it clear that these parameters are text strings (and not variables) that will be replaced by the pre-processor.

Also, note that this style is used other places in the code base by other core devs.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland approved these changes Oct 25, 2023
View reviewed changes
@erlend-aasland
erlend-aasland merged commit f630494 into python:main Oct 25, 2023
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 25, 2023
@erlend-aasland erlend-aasland added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Oct 25, 2023
@miss-islington-app

miss-islington-app Bot commented Oct 25, 2023

Copy link
Copy Markdown

Thanks @sobolevn for the PR, and @erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app

miss-islington-app Bot commented Oct 25, 2023

Copy link
Copy Markdown

Thanks @sobolevn for the PR, and @erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app

miss-islington-app Bot commented Oct 25, 2023

Copy link
Copy Markdown

Sorry, @sobolevn and @erlend-aasland, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker f6304949bb9937e798ecac8b414606dc01bc6d3c 3.11
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app miss-islington-app Bot assigned erlend-aasland Oct 25, 2023
@miss-islington-app

miss-islington-app Bot commented Oct 25, 2023

Copy link
Copy Markdown

Sorry, @sobolevn and @erlend-aasland, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker f6304949bb9937e798ecac8b414606dc01bc6d3c 3.12
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Oct 25, 2023

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD14 3.x has failed when building commit f630494.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1232/builds/358) and take a look at the build logs.
  4. Check if the failure is related to this commit (f630494) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1232/builds/358

Failed tests:

  • test_interpreters

Summary of the results of the build (if available):

==

Click to see traceback logs
remote: Enumerating objects: 16, done.        
remote: Counting objects:   6% (1/16)        
remote: Counting objects:  12% (2/16)        
remote: Counting objects:  18% (3/16)        
remote: Counting objects:  25% (4/16)        
remote: Counting objects:  31% (5/16)        
remote: Counting objects:  37% (6/16)        
remote: Counting objects:  43% (7/16)        
remote: Counting objects:  50% (8/16)        
remote: Counting objects:  56% (9/16)        
remote: Counting objects:  62% (10/16)        
remote: Counting objects:  68% (11/16)        
remote: Counting objects:  75% (12/16)        
remote: Counting objects:  81% (13/16)        
remote: Counting objects:  87% (14/16)        
remote: Counting objects:  93% (15/16)        
remote: Counting objects: 100% (16/16)        
remote: Counting objects: 100% (16/16), done.        
remote: Compressing objects:  11% (1/9)        
remote: Compressing objects:  22% (2/9)        
remote: Compressing objects:  33% (3/9)        
remote: Compressing objects:  44% (4/9)        
remote: Compressing objects:  55% (5/9)        
remote: Compressing objects:  66% (6/9)        
remote: Compressing objects:  77% (7/9)        
remote: Compressing objects:  88% (8/9)        
remote: Compressing objects: 100% (9/9)        
remote: Compressing objects: 100% (9/9), done.        
remote: Total 9 (delta 7), reused 1 (delta 0), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to 'f6304949bb9937e798ecac8b414606dc01bc6d3c'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at f6304949bb gh-111230: Fix errors checking in _ssl module init (#111232)
Switched to and reset branch 'main'

configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Oct 25, 2023

Copy link
Copy Markdown
Member

Indeed, PEP 7 does not specify it explicitly. But all examples in PEP 7 and AFAIK all existing macros write parameter names in lower case. If some macros write it in upper case, it perhaps a new tendency, I never seen this before. I think that it should be specified in PEP 7 explicitly.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Oct 25, 2023

Copy link
Copy Markdown
Member

I created a new discussion for this: https://discuss.python.org/t/all-caps-for-macro-parameter-names/37119.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Oct 26, 2023

Copy link
Copy Markdown
Contributor

I think that it should be specified in PEP 7 explicitly.

I agree.

I created a new discussion for this: https://discuss.python.org/t/all-caps-for-macro-parameter-names/37119.

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hugovk

hugovk commented Nov 9, 2023

Copy link
Copy Markdown
Member

Sorry, @sobolevn and @erlend-aasland, I could not cleanly backport this to 3.11 due to a conflict. Please backport using cherry_picker on command line.

cherry_picker f6304949bb9937e798ecac8b414606dc01bc6d3c 3.11

@sobolevn @erlend-aasland Does this still need backporting? If not, let's close #111230.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 9, 2023

Copy link
Copy Markdown
Member

Taking care of all error paths in an module init function is nice, but I don't think that it's very important to backport. I'm fine with closing the issue, unless @sobolevn is very excited to backport his change :-) @sobolevn: it's up to you ;-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland mentioned this pull request Feb 8, 2024
gh-83004: Clean up refleaks in ssl, socket initialisation #99044
Closed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@sobolevn @aisk
pythongh-111230: Fix errors checking in _ssl module init (python#111232)
23c87cf
Introduce ADD_INT_CONST macro wrapper for PyModule_AddIntConstant()
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@sobolevn @Glyphack
pythongh-111230: Fix errors checking in _ssl module init (python#111232)
fcb01ab
Introduce ADD_INT_CONST macro wrapper for PyModule_AddIntConstant()
@ZeroIntensity ZeroIntensity removed the needs backport to 3.11 only security fixes label Feb 17, 2025
@hugovk hugovk removed the needs backport to 3.12 only security fixes label Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@erlend-aasland erlend-aasland

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@sobolevn @erlend-aasland @bedevere-bot @serhiy-storchaka @hugovk @vstinner @ZeroIntensity
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.