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

replace shell call with a file based detection + user opt in/out #9977

Closed
wants to merge 1 commit into from
Closed

Conversation

DRoppelt
Copy link

@DRoppelt DRoppelt commented May 10, 2022

@DRoppelt
Copy link
Author

DRoppelt commented May 10, 2022

I have no idea how to add a test that could cover this part.

    for(File f : allFilesInLib) {
      if(f.getPath().startsWith("/lib/libc.musl")) return true;
    }
    return false;

I have however verified it manually on an alpine and ubuntu image of adoptium

Alpine

➜  rocksdb git:(main) ✗ docker run -it --entrypoint sh eclipse-temurin:17.0.3_7-jdk-alpine
/ # jshell
May 10, 2022 8:03:23 PM java.util.prefs.FileSystemPreferences$1 run
INFO: Created user preferences directory.
|  Welcome to JShell -- Version 17.0.3
|  For an introduction type: /help intro

jshell> boolean resolveIsMuslLibc() {
   ...>     File[] allFilesInLib = new File("/lib/").listFiles();
   ...>     if(allFilesInLib == null) return false;
   ...>     for(File f : allFilesInLib) {
   ...>       if(f.getPath().startsWith("/lib/libc.musl")) return true;
   ...>     }
   ...>     return false;
   ...>   }
|  created method resolveIsMuslLibc()

jshell> resolveIsMuslLibc()
$2 ==> true

jshell>

Ubuntu

➜  rocksdb git:(main) ✗ docker run -it --entrypoint sh eclipse-temurin:17.0.3_7-jdk
# jshell
May 10, 2022 7:23:08 PM java.util.prefs.FileSystemPreferences$1 run
INFO: Created user preferences directory.
|  Welcome to JShell -- Version 17.0.3
|  For an introduction type: /help intro
jshell> boolean resolveIsMuslLibc() {
   ...>     File[] allFilesInLib = new File("/lib/").listFiles();
   ...>     if(allFilesInLib == null) return false;
   ...>     for(File f : allFilesInLib) {
   ...>       if(f.getPath().startsWith("/lib/libc.musl")) return true;
   ...>     }
   ...>     return false;
   ...>   }
|  created method resolveIsMuslLibc()

jshell> resolveIsMuslLibc()
$6 ==> false

@riversand963
Copy link
Contributor

Hi @adamretter, could you take a look at this sometime?

@adamretter
Copy link
Collaborator

adamretter commented Oct 27, 2022

@DRoppelt I created a new PR which incorporates your work and extends it here - #10889. My goal was to add adopt your feature for allowing the user to specify an option, and your search for musl libc files in lib, whilst preserving the existing approach and adding a small optimisation.

If you are happy with my (our combined PR) in #10889 let me know, and I suggest we close this PR.

@DRoppelt
Copy link
Author

@adamretter LGTM, thank you. For my usecase I would like to see the filebased approach first but I understand that preserving the previous behavior is more important. In a security rescricted env, a failed call to shell might still be flagged. But I can opt-out via the env-var so all good for me 👍

@DRoppelt DRoppelt closed this Oct 28, 2022
facebook-github-bot pushed a commit that referenced this pull request Nov 2, 2022
…rride (#10889)

Summary:
The user may override the detection of whether to use GNU libc (the default) or musl libc by setting the environment variable: `ROCKSDB_MUSL_LIBC=true`.

Builds upon and supersedes: #9977

Pull Request resolved: #10889

Reviewed By: akankshamahajan15

Differential Revision: D40788431

Pulled By: ajkr

fbshipit-source-id: ef594d973fc14cbadf28bfb38434231a18a2107c
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.

4 participants