libnvidia-container: 1.9.0 -> 1.16.2 #347867
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,22 @@ | ||
| From 8799541f99785d2bd881561386676fb0985e939e Mon Sep 17 00:00:00 2001 | ||
| From: Moritz Sanft <[email protected]> | ||
| Date: Thu, 10 Oct 2024 14:32:42 +0200 | ||
| Subject: [PATCH] fix library resolving | ||
|
|
||
| Signed-off-by: Moritz Sanft <[email protected]> | ||
| --- | ||
| src/ldcache.c | 46 +++++++++++++++++----------------------------- | ||
| src/ldcache.h | 2 +- | ||
| src/nvc_info.c | 10 +++------- | ||
| src/nvc_ldcache.c | 2 +- | ||
| 4 files changed, 22 insertions(+), 38 deletions(-) | ||
|
|
||
| diff --git a/src/ldcache.c b/src/ldcache.c | ||
| index 38bab055..8cd30a0f 100644 | ||
| --- a/src/ldcache.c | ||
| +++ b/src/ldcache.c | ||
| @@ -108,40 +108,28 @@ ldcache_close(struct ldcache *ctx) | ||
|
|
||
| int | ||
| ldcache_resolve(struct ldcache *ctx, uint32_t arch, const char *root, const char * const libs[], | ||
| - char *paths[], size_t size, ldcache_select_fn select, void *select_ctx) | ||
|
|
@@ -14,26 +27,17 @@ index 38bab05..e1abc89 100644 | |
| - int override; | ||
| + char dir[PATH_MAX]; | ||
| + char lib[PATH_MAX]; | ||
|
|
||
| - h = (struct header_libc6 *)ctx->ptr; | ||
| memset(paths, 0, size * sizeof(*paths)); | ||
|
|
||
| - for (uint32_t i = 0; i < h->nlibs; ++i) { | ||
| - int32_t flags = h->libs[i].flags; | ||
| - char *key = (char *)ctx->ptr + h->libs[i].key; | ||
| - char *value = (char *)ctx->ptr + h->libs[i].value; | ||
| - | ||
| - if (!(flags & LD_ELF) || (flags & LD_ARCH_MASK) != arch) | ||
| - continue; | ||
| - | ||
| - for (size_t j = 0; j < size; ++j) { | ||
| - if (!str_has_prefix(key, libs[j])) | ||
|
|
@@ -52,14 +56,25 @@ index 38bab05..e1abc89 100644 | |
| - } | ||
| - break; | ||
| - } | ||
| + for (size_t j = 0; j < size; ++j) { | ||
| + snprintf(dir, 100, "@driverLink@/lib"); | ||
| + | ||
| + if (!strncmp(libs[j], "libvdpau_nvidia.so", 100)) | ||
| + strcat(dir, "/vdpau"); | ||
| + snprintf(lib, 100, "%s/%s.%s", dir, libs[j], version); | ||
| + if (path_resolve_full(ctx->err, path, "/", lib) < 0) | ||
| + return (-1); | ||
| + if (!file_exists(ctx->err, path)) | ||
| + continue; | ||
| + | ||
| + paths[j] = xstrdup(ctx->err, path); | ||
| + if (paths[j] == NULL) | ||
| + return (-1); | ||
| } | ||
| return (0); | ||
| } | ||
| diff --git a/src/ldcache.h b/src/ldcache.h | ||
| index 33d78dd7..2b087dbc 100644 | ||
| --- a/src/ldcache.h | ||
| +++ b/src/ldcache.h | ||
| @@ -50,6 +50,6 @@ void ldcache_init(struct ldcache *, struct error *, const char *); | ||
|
|
@@ -68,19 +83,19 @@ index 33d78dd..2b087db 100644 | |
| int ldcache_resolve(struct ldcache *, uint32_t, const char *, const char * const [], | ||
| - char *[], size_t, ldcache_select_fn, void *); | ||
| + char *[], size_t, const char*); | ||
|
|
||
| #endif /* HEADER_LDCACHE_H */ | ||
| diff --git a/src/nvc_info.c b/src/nvc_info.c | ||
| index b7b8adfa..d42f2beb 100644 | ||
| --- a/src/nvc_info.c | ||
| +++ b/src/nvc_info.c | ||
| @@ -217,15 +217,13 @@ find_library_paths(struct error *err, struct dxcore_context *dxcore, struct nvc_ | ||
| if (path_resolve_full(err, path, root, ldcache) < 0) | ||
| return (-1); | ||
| ldcache_init(&ld, err, path); | ||
| - if (ldcache_open(&ld) < 0) | ||
| - return (-1); | ||
|
|
||
| info->nlibs = size; | ||
| info->libs = array_new(err, size); | ||
| if (info->libs == NULL) | ||
|
|
@@ -89,42 +104,44 @@ index 30e3cfd..6d12a50 100644 | |
| - info->libs, info->nlibs, select_libraries_fn, info) < 0) | ||
| + info->libs, info->nlibs, info->nvrm_version) < 0) | ||
| goto fail; | ||
|
|
||
| info->nlibs32 = size; | ||
| @@ -233,13 +231,11 @@ find_library_paths(struct error *err, struct dxcore_context *dxcore, struct nvc_ | ||
| if (info->libs32 == NULL) | ||
| goto fail; | ||
| if (ldcache_resolve(&ld, LIB32_ARCH, root, libs, | ||
| - info->libs32, info->nlibs32, select_libraries_fn, info) < 0) | ||
| + info->libs32, info->nlibs32, info->nvrm_version) < 0) | ||
| goto fail; | ||
| rv = 0; | ||
|
|
||
| fail: | ||
| - if (ldcache_close(&ld) < 0) | ||
| - return (-1); | ||
| return (rv); | ||
| } | ||
|
|
||
| @@ -253,7 +249,7 @@ find_binary_paths(struct error *err, struct dxcore_context* dxcore, struct nvc_d | ||
| char path[PATH_MAX]; | ||
| int rv = -1; | ||
|
|
||
| - if ((env = secure_getenv("PATH")) == NULL) { | ||
| + if ((env = "/run/nvidia-docker/bin:/run/nvidia-docker/extras/bin") == NULL) { | ||
| error_setx(err, "environment variable PATH not found"); | ||
| return (-1); | ||
| } | ||
| diff --git a/src/nvc_ldcache.c b/src/nvc_ldcache.c | ||
| index db3b2f69..ae5def43 100644 | ||
| --- a/src/nvc_ldcache.c | ||
| +++ b/src/nvc_ldcache.c | ||
| @@ -367,7 +367,7 @@ nvc_ldcache_update(struct nvc_context *ctx, const struct nvc_container *cnt) | ||
| if (validate_args(ctx, cnt != NULL) < 0) | ||
| return (-1); | ||
|
|
||
| - argv = (char * []){cnt->cfg.ldconfig, "-f", "/etc/ld.so.conf", "-C", "/etc/ld.so.cache", cnt->cfg.libs_dir, cnt->cfg.libs32_dir, NULL}; | ||
| + argv = (char * []){cnt->cfg.ldconfig, "-f", "/tmp/ld.so.conf.nvidia-host", "-C", "/tmp/ld.so.cache.nvidia-host", cnt->cfg.libs_dir, cnt->cfg.libs32_dir, NULL}; | ||
|
There was a problem hiding this comment. fwiw this is not guaranteed to be writable either /tmp/ld.so.conf.nvidia-host |
||
| if (*argv[0] == '@') { | ||
| /* | ||
| * We treat this path specially to be relative to the host filesystem. | ||
| -- | ||
| 2.46.0 | ||
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly fail to remember what the difference was, but I'll just dump this here for reference: https://github.com/NixOS/nixpkgs/pull/279235/files#diff-2b4dc4504c07052fdeb991c058ab1cd1b3fc215f2475fddab960ebea2db772e7R94-R98. With the original patch, does libnvidia-container still run ldconfig in non-nixos environments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would. But I think it's correct that it doesn't. On non NixOS-systems, we'd probably don't want the impurities caused by this neither, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of drivers we actually kind of do, because they usually aren't at the nixos' predictable location. The dynamic loading behaviour we want pretty much for all apps is:
@driverLink@/lib)/etc/ld.so.{cache,conf}but ensure some kind of isolation (cf. e.g. the libcapsule discussions elsewhere on github and on matrix). We never implemented this last bit in any consistent manner because complications, but in principle it's something to considerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think I need to give this another go to actually understand it more thoroughly. The current patch should have the exact same behaviour as the old one, so it would at least not cause any further regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a more thorough review and think that
ldconfigis ran on all systems (NixOS and non-NixOS), but the system cache (i.e./etc/ld.so.confand/etc/ld.so.cache) are not used. Instead, the special/tmp/ld.so.conf.nvidia-hostand/tmp/ld.so.cache.nvidia-hostdirectories are used for the resolving of nvidia libraries. This should (under the assumption of nothing else using this cache) not cause any impurities on either system. This essentially ensures the precedence of steps 0 through 2 in your list, but not 3. I would also advocate to not implement step 3 here, as I really wouldn't like to maintain such a complex patch (for a library which is unnecessarily complex already).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this definitely wouldn't be implement here, it's a much larger scale effort.
I think for libcuda.so on non-NixOS we do want an impurity, but let's merge this PR regardless and address failures as they come