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

bpo-41798: Allocate the _curses._C_API on the heap memory. #24186

Merged
merged 3 commits into from
Jan 22, 2021
Merged

bpo-41798: Allocate the _curses._C_API on the heap memory. #24186

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
589307b
Allocate the _curses._C_API on the heap memory.
shihai1991 Jan 4, 2021
05785e1
apply victor's comment
shihai1991 Jan 21, 2021
f7af446
apply victor's comment
shihai1991 Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 30 additions & 9 deletions Modules/_cursesmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4705,22 +4705,23 @@ static struct PyModuleDef _cursesmodule = {
NULL
};

static void
curses_destructor(PyObject *op)
{
void *ptr = PyCapsule_GetPointer(op, PyCurses_CAPSULE_NAME);
Py_DECREF(*(void **)ptr);
PyMem_Free(ptr);
}

PyMODINIT_FUNC
PyInit__curses(void)
{
PyObject *m, *d, *v, *c_api_object;
static void *PyCurses_API[PyCurses_API_pointers];

/* Initialize object type */
if (PyType_Ready(&PyCursesWindow_Type) < 0)
return NULL;

/* Initialize the C API pointer array */
PyCurses_API[0] = (void *)&PyCursesWindow_Type;
PyCurses_API[1] = (void *)func_PyCursesSetupTermCalled;
PyCurses_API[2] = (void *)func_PyCursesInitialised;
PyCurses_API[3] = (void *)func_PyCursesInitialisedColor;

/* Create the module and add the functions */
m = PyModule_Create(&_cursesmodule);
if (m == NULL)
Expand All @@ -4732,9 +4733,29 @@ PyInit__curses(void)
return NULL;
ModDict = d; /* For PyCurses_InitScr to use later */

void **PyCurses_API = PyMem_Calloc(PyCurses_API_pointers, sizeof(void *));
Copy link
Member

Choose a reason for hiding this comment

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

This API is weird. Would you be interested to add a structure of 4 pointers? Something like PyDateTime_CAPI.

The 4 pointers are used by:

#define PyCursesWindow_Type (*(PyTypeObject *) PyCurses_API[0])
#define PyCursesSetupTermCalled  {if (! ((int (*)(void))PyCurses_API[1]) () ) return NULL;}
#define PyCursesInitialised      {if (! ((int (*)(void))PyCurses_API[2]) () ) return NULL;}
#define PyCursesInitialisedColor {if (! ((int (*)(void))PyCurses_API[3]) () ) return NULL;}

Maybe you can name the 4 members as:

PyTypeObject *Window_Type
int SetupTermCalled(void)
int Initialised(void)
int InitialisedColor(void)

Or I can merge this PR and then you work on a new PR to add a structure. As you want, tell me what do you prefer ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Or I can merge this PR and then you work on a new PR to add a structure. As you want, tell me what do you prefer ;-)

Oh, I perfer to use another PR to add this structure in this weekend :)

if (PyCurses_API == NULL) {
PyErr_NoMemory();
return NULL;
}
/* Initialize the C API pointer array */
PyCurses_API[0] = (void *)Py_NewRef(&PyCursesWindow_Type);
PyCurses_API[1] = (void *)func_PyCursesSetupTermCalled;
PyCurses_API[2] = (void *)func_PyCursesInitialised;
PyCurses_API[3] = (void *)func_PyCursesInitialisedColor;

/* Add a capsule for the C API */
c_api_object = PyCapsule_New(PyCurses_API, PyCurses_CAPSULE_NAME, NULL);
PyDict_SetItemString(d, "_C_API", c_api_object);
c_api_object = PyCapsule_New(PyCurses_API, PyCurses_CAPSULE_NAME,
curses_destructor);
if (c_api_object == NULL) {
Py_DECREF(PyCurses_API[0]);
PyMem_Free(PyCurses_API);
Copy link
Member

Choose a reason for hiding this comment

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

Hum, there is no a refleak here. Does it work to call curses_destructor() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, thanks, updated.
Weird. ./python Lib/test/regrtest.py -ucurses test_curses -vv -R 3:3 can not catch this refleak.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it work to call curses_destructor() here?

No, it's can not work because capsule object haven't created

Copy link
Member

Choose a reason for hiding this comment

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

Weird. ./python Lib/test/regrtest.py -ucurses test_curses -vv -R 3:3 can not catch this refleak.

test_curses doesn't test memory allocation failures. See _testcapi.set_nomemory() for that.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, and _curses doesn't use multiphase init API yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it.

return NULL;
}
if (PyDict_SetItemString(d, "_C_API", c_api_object) < 0) {
Py_DECREF(c_api_object);
return NULL;
}
Py_DECREF(c_api_object);

/* For exception curses.error */
Expand Down