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

bpo-36073: Raise ProgrammingError on recursive usage of cursors in sqlite converters #11984

Closed
wants to merge 1 commit into from

Conversation

sir-sigurd
Copy link
Contributor

@sir-sigurd sir-sigurd commented Feb 22, 2019

@BoboTiG
Copy link
Contributor

BoboTiG commented Jun 21, 2019

Thanks @sir-sigurd, I think a NEWs entry would be interesting, don't you?

@sir-sigurd sir-sigurd requested a review from berkerpeksag as a code owner June 22, 2019 05:22
@sir-sigurd sir-sigurd force-pushed the sqlite-converter-segfault branch 2 times, most recently from 16a6d90 to 6f1b983 Compare June 22, 2019 10:27
@erlend-aasland
Copy link
Contributor

@sir-sigurd, can you rebase onto master?

Comment on lines 365 to 366
return check_cursor_locked(cur) &&
pysqlite_check_thread(cur->connection) && pysqlite_check_connection(cur->connection);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please format these lines according to PEP 7.

static int check_cursor_locked(pysqlite_Cursor* cur)
{
if (cur->locked) {
PyErr_SetString(pysqlite_ProgrammingError, "Recursive use of cursors not allowed.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Line is longer than 79 characters, ref. PEP 7.

Modules/_sqlite/cursor.c Outdated Show resolved Hide resolved

def suite():
regression_suite = unittest.makeSuite(RegressionTests, "Check")
return unittest.TestSuite((
regression_suite,
unittest.makeSuite(ConverterProgrammingErrorTestCase),
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be rebased onto master in order to fit in with the changes in the test suite.

@sir-sigurd
Copy link
Contributor Author

@erlend-aasland
I'll try to rebase in the near future, though I almost do not remember anything about this.

@erlend-aasland
Copy link
Contributor

I'll try to rebase in the near future, though I almost do not remember anything about this.

Thanks, there's no hurry :)

@sir-sigurd sir-sigurd force-pushed the sqlite-converter-segfault branch from d3a828d to c79e784 Compare April 12, 2021 08:14
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

LGTM!

@erlend-aasland
Copy link
Contributor

@serhiy-storchaka would you mind reviewing this?

@erlend-aasland
Copy link
Contributor

Could you rebase again (bco. GH-27884), @sir-sigurd? :)

Also, please clean up the regression test:

  • line length is limited to 79 chars
  • please use double quotes

@sir-sigurd
Copy link
Contributor Author

Closed in favor of GH-29054.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants