cmd: make locking around namespaces explicit #3149
Conversation
This patch adds a helper function for calling something while holding an exclusive filesystem lock. The lock can be scoped to a specific snap or global. 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>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The sanity timeout APIs belong logically with the locking primitives so move them there. In addition, since they are needed in the namespace module, make them public. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The namespace module now relies the caller to use locking primitives for all the locking operations. 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>
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>
| gint exit_status = 0; | ||
| GError *error = NULL; | ||
|
|
||
| argv = calloc(5, sizeof *argv); |
There was a problem hiding this comment.
What is the advantage of doing this (and the following 14 lines) over:
const char *argv[] = { "rm", "-rf", "--", dir, NULL };
There was a problem hiding this comment.
Arguments need to be writable.
There was a problem hiding this comment.
What about: char *argv[] = { "rm", "-rf", "--", (char*)dir, NULL }; then (especially since execv will not modify them, according to http://pubs.opengroup.org/onlinepubs/009604499/functions/exec.html). Anyway, one of the reasons why C code reviews make me grumpy.
There was a problem hiding this comment.
I mean that individual arguments need to be writable according to the API. That's why all the strdup's are up there.
There was a problem hiding this comment.
The opengroup page is an interessting read (the RATIONAL part about argv) to see why argv is char*. i.e. The statement about argv[] and envp[] being constants is included to make explicit to future writers of language bindings that these objects are completely constant. Due to a limitation of the ISO C standard, it is not possible to state that idea in standard C.. But shrug things like this make me do as little as possible with C code.
| return lock_fd; | ||
| } | ||
|
|
||
| int sc_lock_global() |
There was a problem hiding this comment.
I guess this also needs a sc_unlock_global() :)
cmd/snap-confine/snap-confine.c
Outdated
| sc_initialize_ns_groups(); | ||
| // TODO: implement this. | ||
| debug("share snap directory here..."); | ||
| sc_unlock(NULL, global_lock_fd); |
There was a problem hiding this comment.
I would prefer sc_unlock_global() here.
| struct sc_ns_group *group = | ||
| sc_open_ns_group(snap_name, SC_NS_FAIL_GRACEFULLY); | ||
| if (group != NULL) { | ||
| sc_lock_ns_mutex(group); |
There was a problem hiding this comment.
It seems cmd/snap-confine/ns-support.h still has a reference in the header to this one?
There was a problem hiding this comment.
Ah, thanks, I'll check if there's anything else stale there.
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>
|
The errors show some issues in the CI setup. We're executing snap-confine under a profile that doesn't have the correct rules but those rules are now present in the tree. Probably the run-from-core logic is faulty somewhere. I'll investigate this now. |
There was a problem hiding this comment.
Looks good (but needs a second review). I still dislike https://github.com/snapcore/snapd/pull/3149/files#r110140992 but shrug its just tests.
This branch allows anyone to use new locking primitives (sc_lock/sc_unlock) and
makes the namespace module not do any locking anymore. Locking is now moved
explicitly to happen in snap-confine so that we can do various initialization
under one global lock and then some more initialization under per-snap lock.
This will allow us to move the sharing of /snap to inside snap-confine and have
support for running inside containers and running with deputy systemd.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com