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

snap-confine: Only attempt to copy/mount NVIDIA libs when NVIDIA is used #3975

Merged
merged 1 commit into from
Sep 29, 2017

Conversation

ikeydoherty
Copy link
Contributor

@ikeydoherty ikeydoherty commented Sep 27, 2017

This hasn't yet presented an issue in full confinement environments,
as the only fully confined environment available is Ubuntu, and the snaps
being tested are also built using the Ubuntu libraries.

This change will now only make the NVIDIA libraries available if the nvidia
kernel module is loaded and present, to ensure that confined snaps running
on open source drivers will not run into any linking issues by exposing the
host GL library into the runtime.

Signed-off-by: Ikey Doherty ikey@solus-project.com

This hasn't _yet_ presented an issue in full confinement environments,
as the only fully confined environment available is Ubuntu, and the snaps
being tested are also built using the Ubuntu libraries.

This change will now only make the NVIDIA libraries available if the nvidia
kernel module is loaded and present, to ensure that confined snaps running
on open source drivers will not run into any linking issues by exposing the
host GL library into the runtime.

Signed-off-by: Ikey Doherty <ikey@solus-project.com>
Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

+1

@codecov-io
Copy link

codecov-io commented Sep 27, 2017

Codecov Report

Merging #3975 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3975      +/-   ##
==========================================
+ Coverage   75.97%   75.98%   +<.01%     
==========================================
  Files         423      423              
  Lines       36505    36505              
==========================================
+ Hits        27734    27737       +3     
+ Misses       6833     6830       -3     
  Partials     1938     1938
Impacted Files Coverage Δ
overlord/ifacestate/helpers.go 63% <0%> (+0.66%) ⬆️
cmd/snap/cmd_aliases.go 95% <0%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d7d79c...5aef05f. Read the comment docs.

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

This is great, thank you. One question inline, basicly I'm wondering if the detection method is stable over time.

@@ -33,6 +33,8 @@
#include "../libsnap-confine-private/string-utils.h"
#include "../libsnap-confine-private/utils.h"

#define SC_NVIDIA_DRIVER_VERSION_FILE "/sys/module/nvidia/version"
Copy link
Contributor

Choose a reason for hiding this comment

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

How stable is this name? For how long is iit available?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are already using it and it is present in all the driver versions out there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya I just moved the existing define in the file to be available where I need it, but the name is stable for loaded kernel module.

@zyga zyga merged commit 7c759ae into canonical:master Sep 29, 2017
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