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

[TOOLS-4438] replace tinfo.so.x LINK to dynamic library loading at runtime (dlopen) #1

Merged
merged 5 commits into from
Mar 16, 2023

Conversation

kisoo-han
Copy link
Contributor

@kisoo-han kisoo-han commented Mar 15, 2023

http://jira.cubrid.org/browse/TOOLS-4438

Purpose

  • replace libtinfo.so link to runtime dynamic library loading by dlopen ()

Implementation

Remarks

  • idea & original code was written by @mhoh3963
  • it is for removing curses major version dependancy

@kisoo-han kisoo-han requested review from mhoh3963 and airnet73 March 15, 2023 06:11
@kisoo-han kisoo-han self-assigned this Mar 15, 2023
src/terminal.c Outdated
if (dl_handle != NULL) {
dlclose(dl_handle);
dl_handle = NULL;
} }

Choose a reason for hiding this comment

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

There is one more brace "}".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment!
I missed it. Probably mistake from ^C/^V action.

@kisoo-han kisoo-han changed the title [TOOLS-4438] replace tinfo.so LINK to dynamic library loading at runtime (dlopen) [TOOLS-4438] replace tinfo.so.x LINK to dynamic library loading at runtime (dlopen) Mar 15, 2023
src/terminal.c Outdated
char *(*tgetstr) (char *id, char **area);
int (*tputs) (const char *str, int affcnt, int (*putc)(int));

#define NCURSES_HIGH_VERSION 10

Choose a reason for hiding this comment

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

How about changing the prefix name from NCURSES to TINFOLIB ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks better.
Thank you!

src/terminal.c Outdated
}

if (dl_handle == NULL){
fprintf (stderr, "ERROR: Cannot load ncurses library.\n");

Choose a reason for hiding this comment

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

I think you need to tell the user the exact library name you need. So it would be nice to change the error message as the following.
"ERROR : Cannot load tinfo library. Please install tinfo library.\n"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that installing ncurses would also install tinfo.
I'll change that, thank you for your comment!

src/terminal.c Outdated

if (dl_handle == NULL){
fprintf (stderr, "ERROR: Cannot load ncurses library.\n");
goto fail1;

Choose a reason for hiding this comment

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

If an error is returned when the library cannot be loaded or the corresponding function is not found, segfault may occur due to incorrect error handling at the calling function.
It seems better to exit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't considered that.
You right.
Errors related to dlopen () and dlsym () cannot be recovered, and result in coredump.
So _exit should be used.

@kisoo-han kisoo-han requested a review from mhoh3963 March 16, 2023 03:32
@kisoo-han kisoo-han merged commit 91d08f2 into CUBRID:master Mar 16, 2023
@kisoo-han kisoo-han deleted the tools-4438-runtime-loading-tinfo branch March 16, 2023 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants