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

cmd/snap-confine: introduce sc_invocation #6571

Merged
merged 4 commits into from Mar 7, 2019

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented Mar 6, 2019

Currently passing data around snap-confine is not very easy because of
historically organic growth of the codebase. To simplify some of that
I'd like to introduce sc_invocation. An ad-hoc structure that captures
some of the variables that participate in the essential part of
snap-confine's logic.

The purpose of this patch is to introduce this structure, switch some of
the code to use it (as opposed to accessing the local variables) and
set the stage for a subsequent patch that moves a large chunk of code
into the two new stubs: enter_{,non_}classic_execution_environment().

This should eventually allow us to make a single call to
sc_should_use_normal_mode() and simply pass the result around via the
invocation structure. There is plenty of more refactoring that can be
done after this patch is in place.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

Currently passing data around snap-confine is not very easy because of
historically organic growth of the codebase. To simplify some of that
I'd like to introduce sc_invocation. An ad-hoc structure that captures
some of the variables that participate in the essential part of
snap-confine's logic.

The purpose of this patch is to introduce this structure, switch some of
the code to use it (as opposed to accessing the local variables) and
set the stage for a subsequent patch that moves a large chunk of code
into the two new stubs: enter_{,non_}classic_execution_environment().

This should eventually allow us to make a single call to
sc_should_use_normal_mode() and simply pass the result around via the
invocation structure. There is plenty of more refactoring that can be
done after this patch is in place.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Handling uid/gid is a bit more delicate as those values routinely change
thought execution. Instead of passing them around in the invocation
structure pass them explicitly to the only place that needs it for now.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@codecov-io
Copy link

Codecov Report

Merging #6571 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6571      +/-   ##
=========================================
- Coverage   79.13%   79.1%   -0.04%     
=========================================
  Files         580     580              
  Lines       45089   45089              
=========================================
- Hits        35681   35667      -14     
- Misses       6509    6521      +12     
- Partials     2899    2901       +2
Impacted Files Coverage Δ
httputil/useragent.go 77.14% <0%> (-5.72%) ⬇️
cmd/snap-seccomp/main.go 61.13% <0%> (-3.63%) ⬇️
overlord/hookstate/ctlcmd/services.go 76.66% <0%> (-3.34%) ⬇️
daemon/api_connections.go 91.78% <0%> (-2.06%) ⬇️
cmd/snap/cmd_aliases.go 91.37% <0%> (-1.73%) ⬇️
overlord/snapstate/snapstate.go 81.6% <0%> (-0.17%) ⬇️
overlord/ifacestate/handlers.go 63.98% <0%> (ø) ⬆️
overlord/hookstate/hookmgr.go 74.51% <0%> (+0.96%) ⬆️

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 e1b5d24...0c22713. Read the comment docs.

@pedronis
Copy link
Collaborator

pedronis commented Mar 6, 2019

we discussed using sc_invocation only in the non-classic path for now

zyga added 2 commits March 7, 2019 12:20
After a good night's sleep I realised I should not have put it there
in the first place. This will allow the invocatoin to collect and contain
only the parts of the arguments that are related to the invocation of
snap-confine and leave out certain things that have more complex state
(uids) or are an implementation detail (apparmor struct) out of it.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
…nvironment

The classic path is (for now) empty so let's not use the invocation
there just yet.


Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you

Copy link
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

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

+1

const char *base_snap_name;
const char *security_tag;
const char *snap_instance;
} sc_invocation;
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat!

@zyga zyga merged commit b619eea into snapcore:master Mar 7, 2019
@zyga zyga deleted the tweak/refactor-sc-main-1 branch March 7, 2019 20:58
@zyga zyga added this to In progress in Core16 using pivoting mount namespace via automation Mar 18, 2019
@zyga zyga moved this from In progress to Done in Core16 using pivoting mount namespace Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants