cmd/snap-confine: pass sc_invocation instead of numerous args around #6575
Conversation
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
Since we want to start taking advantage of the invocation being passed around, instead of the numerous separate arguments, we need to make it public and something that can be included in other places. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
This completes the set of facts we ought to be presented about an invocation. All the things that come from the command line arguments and the environment are now present there. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
sc_join_preserved_ns used to take three values that are now represented in sc_invocation, this simplifies the function call. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
…stale_ns This removes three arguments and replaces them with one. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
This replaces three arguments with one. Internally there's a complication because the function makes a local modification to base_snap_name for the ubuntu-core fallback case. This property is preserved to be tackled separately. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
pedronis
reviewed
Mar 7, 2019
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This will allow us to remove the locals that are duplicated in main. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The new helper function handles initialization of sc_invocation, which mostly involves internal consistency checks. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Drop the inline copy of sc_init_invocation and the associated local variables from main and replace them with the use call to library function. This also ensures that there is no duplication of what is the reference value of any of the key properties. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The file is brand new and we can still switch without killing relevant history. This mainly gives us less strict line length and less odd line breaks. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
|
I accidentally pushed bits from -5 here, sorry about that (I meant to split it off for easier review before). Specifically those patches: |
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
zyga
force-pushed
the
tweak/refactor-sc-main-4
branch
from
64c9d01
to
2e29bf0
Compare
pedronis
reviewed
Mar 12, 2019
pedronis
reviewed
Mar 12, 2019
jdstrand
suggested changes
Mar 12, 2019
By popular demand, sc_invocation now holds a copy of the memory it used to reference. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The abbreviated form is a GCC extension, don't use it. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
….[ch] Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
zyga
moved this from In progress
to Review in progress
in Core16 using pivoting mount namespace
|
@pedronis I had to undo the meld into |
In subsequent branch, unit tests fail to link because mount-support-tests.c depend on functions related to sc_invocation that were defined in snap-confine.c. Therefore move them to snap-confine-invocation.[ch] like they used to be. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
I was meaning to do it later but I realised we must do it now, because we sc_strdup() the executable. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
|
@pedronis in the -5 branch. |
|
Hmm, travis doesn't seem to see this branch https://travis-ci.org/snapcore/snapd/builds/507790331?utm_source=github_status&utm_medium=notification |
Core16 using pivoting mount namespace
automation
moved this from Review in progress
to Reviewer approved
Mar 19, 2019
|
@pedronis no, because I pushed the fixed version after merging locally. The problem is simple, mount-support depends on some of the functions defined in snap-confine.c and that cannot be merged into unit tests due to clashes with main. We could use ifdef tricks but I think that's less desirable. |
mvo5
reviewed
Mar 20, 2019
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The helper has no uses yet and doesn't need to exist as a public function. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The error message for unset SNAP_INSTANCE_NAME environment variable is the only user interaction printed when snap-confine is invoked as a program without any arguments. As such it is important that the error message at least hints towards the solution of the problem. At the same time the internals of sc_init_invocation() don't require the value to come from the environment so it is sensible to use a dedicated error message there. This patch does both. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
mvo5
reviewed
Mar 20, 2019
The _fini function is a destructor that does not concern itself with heap or stack or bss allocation of the principal object. The cleanup function is a combination of object destruction and memory deallocation applicable for a GCC cleanup attribute extension which necessarily marries the two concepts. It was raised that in this case, of a stack allocated object, there is no difference between the destructor and the cleanup function. As such I will fold them into one function. Separately, I will do a pass over snap-confine's memory handling to unify it and give it a common naming scheme and propose the changes that will be consistent across the tree. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
mvo5
approved these changes
Mar 21, 2019
Core16 using pivoting mount namespace
automation
moved this from Reviewer approved
to Done
Mar 21, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Several places use a combination of base_snap_name, snap_name (or
snap_instance, those are used interchangeably) and normal_mode flag and pass
them around as separate arguments.
With the recent introduction of sc_invocation as a private feature of
snap-confine, we can now take advantage of the fact that it can tidy up much of
this complexity. This is done without impacting behaviour in any way.
I highlighted the special case of ubuntu-core fallback logic modifying the base
snap name behind the back of the rest of the code. This is not a new property
so I kept it intact for now
Signed-off-by: Zygmunt Krynicki me@zygoon.pl