Skip to content

Introduce uv_dlopen_with_flags()#1331

Closed
ezequielgarcia wants to merge 1 commit intolibuv:v1.xfrom
ezequielgarcia:dlopen_with_flags
Closed

Introduce uv_dlopen_with_flags()#1331
ezequielgarcia wants to merge 1 commit intolibuv:v1.xfrom
ezequielgarcia:dlopen_with_flags

Conversation

@ezequielgarcia
Copy link
Copy Markdown

This is a new try at extending uv_dlopen to support UNIX dlopen flags. Previous try was rejected and closed: #635.

@saghul reasons for closing were:

Closing. Current behavior is in use in the wild and we cannot break it. Also, since this has no applicability on Windows, you could use the dl* functions directly, libuv doesn't do anything special about them anyway.

First objection is addressed by introducing a new function, uv_dlopen_with_flags. This is a no-op in Windows.

Now, about the Windows behavior objection. Introducing a uv_dlopen_with_flags function makes portable programs (such as node.js) result in much cleaner code. In other words, we would like to simply call uv_dlopen_with_flags without any ugly platform specific ifdef'ery.

See ezequielgarcia/node@3820c96 for an example on how this function would be used.

It's important to mention that uv_dlopen implementation is maintained, keeping backwards compatibility.

Not surprisingly, this new function is used to pass non-default flags to dlopen.
To keep backwards compatibility, uv_dlopen() is maintained, which calls
dlopen with RTLD_LAZY.

For example, this function will be useful when adding support for dlopen
flags in Node.js.

Note that uv_dlopen_with_flags() is a no-op in windows, which just
calls uv_dlopen().

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
@saghul
Copy link
Copy Markdown
Member

saghul commented May 1, 2017

I'm still -1. IF you need Unix only support you can use dlopen(3) directly. Node already ships with a bunch of native methods, this one could live there too.

@bnoordhuis
Copy link
Copy Markdown
Member

I must admit I don't really see the point either. It doesn't add anything over dlopen() since you have to pass in platform-specific flags anyway. Why not call dlopen() directly in that case?

@ezequielgarcia
Copy link
Copy Markdown
Author

Fair enough. However, it does seem a little short sight to provide support for dlopen and then force a given flag such as RTLD_LAZY. A quick google search shows that these flags are rather fundamental and many common uses of dlopen mandate using them.

@ezequielgarcia
Copy link
Copy Markdown
Author

ezequielgarcia commented May 1, 2017

@saghul @bnoordhuis While working on implementing this in Node exclusively, I came across tihs: https://github.com/dlfcn-win32/dlfcn-win32/blob/master/dlfcn.c#L295. It shows a way to give meaning to RTLD_GLOBAL in Windows.

@cjihrig
Copy link
Copy Markdown
Contributor

cjihrig commented May 15, 2017

This appears to have moved to nodejs/node, so I'll close here.

@cjihrig cjihrig closed this May 15, 2017
refack pushed a commit to nodejs/node that referenced this pull request Sep 8, 2017
* add constants for dlopen flags, which are needed
  for dlopen's flag passing.

* introduce an optional parameter for process.dlopen(),
  allowing to pass dlopen flags (using values from os.constants.dlopen).

If no flags are passed, the default behavior is to load the library
with RTLD_LAZY (perform lazy binding) and RTLD_LOCAL (symbols are
available only locally).

PR-URL: #12794
Refs: #4105
Refs: libuv/libuv#1331
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
* add constants for dlopen flags, which are needed
  for dlopen's flag passing.

* introduce an optional parameter for process.dlopen(),
  allowing to pass dlopen flags (using values from os.constants.dlopen).

If no flags are passed, the default behavior is to load the library
with RTLD_LAZY (perform lazy binding) and RTLD_LOCAL (symbols are
available only locally).

PR-URL: nodejs#12794
Refs: nodejs#4105
Refs: libuv/libuv#1331
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
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.

4 participants