interfaces: drop udev tagging from framebuffer interface #3089

Merged
merged 3 commits into from Apr 3, 2017

Conversation

Projects
None yet
7 participants
Contributor

chunsangjeong commented Mar 28, 2017

This will fix access denied of opengl interface when it's used with framebuffer
interface in the same snap. It's a first step of fix which allow us to unblock
the referenced case immediately and the proper udev tagging will be included in
a second step.

Fixes: https://bugs.launchpad.net/snapd/+bug/1675738
Signed-off-by: Chunsang Jeong chunsang.jeong@canonical.com

Contributor

zyga commented Mar 28, 2017

I have merged master into this branch, resolved conflicts resulting from new snapd APIs and pushed this back to https://github.com/zyga/snapd/tree/fix-opengl-interface-udev-tag-dev-fb. I also contacted @chunsangjeong to make sure he is aware of this but I was unable to push into the pull request myself. Once the PR is updated it should be ready for review.

interfaces/builtin/framebuffer.go
@@ -83,17 +82,6 @@ func (iface *FramebufferInterface) AppArmorConnectedPlug(spec *apparmor.Specific
// Getter for the security snippet specific to the plug
func (iface *FramebufferInterface) ConnectedPlugSnippet(plug *interfaces.Plug, slot *interfaces.Slot, securitySystem interfaces.SecuritySystem) ([]byte, error) {
- switch securitySystem {
@femdom

femdom Mar 28, 2017

Contributor

Looks better now=)

Does it mean that udev tagging will never be enabled for the framebuffer interface?

@zyga

zyga Mar 28, 2017

Contributor

I think we will do a pass and enable it for everything. Now the problem is that any interface using udev tagging breaks every interface not using it that needs to access devices.

@morphis

morphis Mar 28, 2017

Contributor

We have a work item to convert all interfaces to use proper udev tagging for our next sprint which will then land in a future snapd version 2.24/2.25/2.26 depending on complexity and timing.

Contributor

morphis commented Mar 29, 2017

LGTM

Instead of just removing all the code, could we comment out the code and add a TODO comment that we are not doing this due to the bug and that we'll be reintroducing the udev tagging soon? This will help us remember to get back to this....

+1 on the approach and the effect to workaround the issue temporarily.

Contributor

chunsangjeong commented Mar 30, 2017

Added TODO comment and re-pulled https://github.com/zyga/snapd/tree/fix-opengl-interface-udev-tag-dev-fb. @zyga please check the updated branch.

Contributor

femdom commented Mar 30, 2017

Thanks! I am going to test this today in the next 8 hours.

Contributor

chunsangjeong commented Mar 30, 2017

@zyga I just rebased with the latest code and re-commited from there. Please check if it still conflicts with your branch.Thanks.

Contributor

jdstrand commented Mar 30, 2017

Approving, but please adjust framebuffer_test.go accordingly (then your tests should start passing).

This will fix access denied of opengl interface when it's used with f…
…ramebuffer

interface in the same snap. It's a first step of fix which allow us to unblock
the referenced case immediately and the proper udev tagging will be included in
a second step.

Fixes: https://bugs.launchpad.net/snapd/+bug/1675738
Signed-off-by: Chunsang Jeong <chunsang.jeong@canonical.com>
Contributor

morphis commented Mar 31, 2017

LGTM

zyga approved these changes Mar 31, 2017

+1

femdom approved these changes Mar 31, 2017

I tested the snapd on our system and was able to access the /dev/vchiq just ok.

zyga and others added some commits Mar 31, 2017

Fix formatting
Signed-off-by: Chunsang Jeong <chunsang.jeong@canonical.com>

@mvo5 mvo5 added this to the 2.24 milestone Apr 3, 2017

@zyga zyga merged commit 85c9855 into snapcore:master Apr 3, 2017

1 of 6 checks passed

xenial-amd64 autopkgtest finished (failure)
Details
xenial-ppc64el autopkgtest finished (failure)
Details
zesty-amd64 autopkgtest finished (failure)
Details
xenial-i386 autopkgtest running
Details
yakkety-amd64 autopkgtest running
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Nice! Merged! Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment