cmd/{snap-confine,libsnap-confine-private,snap-shutdown}: cleanup low-level C bits #4153

bboozzoo · 2017-11-06T10:02:47Z

This is a series of cleanups for low level C bits. Notable changes is that warning flags are more strict than they used to be. Namely -Wextra is included now (-Wall is only half of warnings one usually wants) plus some exceptions for warnings that gcc and clang are a bit over eager to raise.

The cleanups come from building with warning flags enabled, and separate runs with clang 5.0.0 and sparse 0.5.1.

I've also ran some tests locally with asan and ubsan. Both were ok.

This is only the first part of cleanups. We should aim to integrate at least clang builds into the CI pipeline.

Bump the number of warning issue by compiler. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

… symbol takes no paramters, make symbols static Make sure to use void in parameter list when a function takes no paramters. Otherwise the symbol can take any number of parameters. Where possible, make symbols static. Fixes 'function declaration isn’t a prototype [-Wstrict-prototypes]' warnings. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Reformat the F() macro to make it readable. Make sure to include braces braces in do-while statement. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Nested functions are not supported outside of gcc. Refactor the code using this feature. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

codecov-io · 2017-11-06T10:59:03Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #4153      +/-   ##
==========================================
- Coverage   75.38%   75.38%   -0.01%     
==========================================
  Files         435      435              
  Lines       37724    37724              
==========================================
- Hits        28439    28437       -2     
- Misses       7296     7298       +2     
  Partials     1989     1989

Impacted Files	Coverage Δ
overlord/ifacestate/helpers.go	`59.6% <0%> (-0.67%)`	⬇️

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 4839a7d...f352040. Read the comment docs.

…sts are gcc specific Rename the test to indicate that it is GCC specific Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Introduce a test that builds the low-level C bits with clang and runs unit tests. This should ensure code can also be built with clang and we get an early heads up if gcc specific bits are incoming. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga

Small tweak, otherwise LGTM

As discussed on IRC I think this will bitrot unless we go and actually test a clang-based build. I know you are doing that already.

zyga · 2017-11-06T10:05:18Z

cmd/libsnap-confine-private/cleanup-funcs-test.c

@@ -20,15 +20,18 @@

 #include <glib.h>

+static int called = 0;
+
+void cleanup_fn(int *ptr)


please change to static void cleanup_fn(int *ptr)

Thanks. I noticed that CHECK_CFLAGS were not set for libsnap-confine-private.a and its unit tests at all, will be fixing this right now.

zyga · 2017-11-06T11:32:45Z

cmd/Makefile.am

@@ -7,6 +7,8 @@ dist_man_MANS =
 noinst_PROGRAMS =
 noinst_LIBRARIES =

+CHECK_CFLAGS = -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes \


Gentoo has a policy where those are not added automatically as they tend to break good builds due to new compiler releases. I was wondering if those flags should not be in the unit tests, and instead we could resort to perhaps, just, -Wall

Interesting, do you know if they push -Werror by default? Just having the warnings enabled should not cause build failures.

zyga

Two extra suggestions.

zyga · 2017-11-06T11:45:11Z

tests/unit/c-unit-tests-clang/task.yaml

+execute: |
+    # Refresh autotools build system
+    cd "$SPREAD_PATH/cmd/"
+    autoreconf --install --force


Maybe we could just run autogen.sh, this would handle the case of what arguments to pass?

zyga · 2017-11-06T11:45:30Z

tests/unit/c-unit-tests-clang/task.yaml

+    cd "$SPREAD_PATH/cmd/autogarbage"
+    EXTRA_CONF=
+    if [ ! -d /sys/kernel/security/apparmor ]; then
+        EXTRA_CONF="--disable-apparmor --disable-seccomp"


I think we now enable seccomp even if apparmor is not enabled.

This is fixed now, we'll calling autogen.sh so it will set whatever flags we have there for any particular platform.

mvo5

Thanks for taking this on. I added comments inline, I like the fact that the "int" vs "size_t" issues got cleaned up and there are some other nice fixes. I'm not so sure about f() -> f(void). I understand the technical reason behind it but it makes me sad and I wish there was a way to say -std=c11-dont-be-silly to avoid the need for this.

mvo5 · 2017-11-06T11:51:21Z

cmd/libsnap-confine-private/fault-injection-test.c

@@ -33,7 +33,7 @@ static bool broken_alter_msg(struct sc_fault_state *state, void *ptr)
 	return true;
 }

-static void test_fault_injection()
+static void test_fault_injection(void)


I dislike that this needs to be done :/ That is one of my complains about C (one of many ;) - the fact that K&R C from ~1879 requires us to nowdays write clunky (void) like this. Oh well.

I guess you meant 1978, although it does feel like 19th century by modern standards :)

mvo5 · 2017-11-06T12:00:41Z

cmd/libsnap-confine-private/mount-opt.c

@@ -33,7 +33,13 @@ const char *sc_mount_opt2str(char *buf, size_t buf_size, unsigned long flags)
 {
 	unsigned long used = 0;
 	sc_string_init(buf, buf_size);
-#define F(FLAG, TEXT) do if (flags & (FLAG)) { sc_string_append(buf, buf_size, #TEXT ","); flags ^= (FLAG); } while (0)
+
+#define F(FLAG, TEXT) do {                                         \


mvo5 · 2017-11-06T12:02:36Z

cmd/libsnap-confine-private/snap-test.c

@@ -144,7 +144,7 @@ static void test_sc_snap_name_validate()
 		"a0", "a-0", "a-0a",
 		"01game", "1-or-2"
 	};
-	for (int i = 0; i < sizeof valid_names / sizeof *valid_names; ++i) {
+	for (size_t i = 0; i < sizeof valid_names / sizeof *valid_names; ++i) {


mvo5 · 2017-11-06T12:04:25Z

cmd/snap-confine/snap-confine.c

@@ -48,7 +48,7 @@
 // sc_maybe_fixup_permissions fixes incorrect permissions
 // inside the mount namespace for /var/lib. Before 1ccce4
 // this directory was created with permissions 1777.
-void sc_maybe_fixup_permissions()
+static void sc_maybe_fixup_permissions(void)


bboozzoo · 2017-11-06T13:45:15Z

Noticed that there were no CFLAGS being set for libsnap-confine-private.a. Will be pushing a patch shortly.

…unit tests Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…otype warnings Fixes 'function declaration isn’t a prototype [-Wstrict-prototypes]' warnings. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

… unit tests This will allow us to catch warnings as part of CI pipeline Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ariable Allow passing of desired build directory path in BUILD_DIR environment variable. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…ogen.sh Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

ENOENT case is not expected to fall through. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added 6 commits November 6, 2017 10:44

cmd: enable more compiler warnings

c3f581a

Bump the number of warning issue by compiler. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

snap-confine: fix comparison of signed with unsigned

1e407d3

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

libsnap-confine-private: fix comparison of signed with unsigned

668b676

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

libsnap-confine-private: reformat F() macro, include do-while braces

70d6c93

Reformat the F() macro to make it readable. Make sure to include braces braces in do-while statement. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

libsnap-confine-private: do not use nested functions

fd8f5fb

Nested functions are not supported outside of gcc. Refactor the code using this feature. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga changed the title ~~cmd/{snap-confine,libsnap-confine-private,snap-shutdown}: cleanup lowe level C bits~~ cmd/{snap-confine,libsnap-confine-private,snap-shutdown}: cleanup low-level C bits Nov 6, 2017

bboozzoo added 2 commits November 6, 2017 12:36

tests/unit/c-unit-tests: rename to unit-tests-gcc to indicate that te…

706a213

…sts are gcc specific Rename the test to indicate that it is GCC specific Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga approved these changes Nov 6, 2017

View reviewed changes

zyga reviewed Nov 6, 2017

View reviewed changes

mvo5 reviewed Nov 6, 2017

View reviewed changes

bboozzoo added 6 commits November 6, 2017 14:47

cmd: properly set CHECK_CFLAGS for libsnap-confine-private.a and its …

b6d40fc

…unit tests Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

cmd/libsnap-confine-private: cleanup remaining declaration isn't prot…

2ed4577

…otype warnings Fixes 'function declaration isn’t a prototype [-Wstrict-prototypes]' warnings. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

cmd: make enabled C compiler warnings become error when building with…

efd8081

… unit tests This will allow us to catch warnings as part of CI pipeline Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

cmd/autogen: allow passing of build directory through BUILD_DIR env v…

59cfa18

…ariable Allow passing of desired build directory path in BUILD_DIR environment variable. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

tests/unit/c-unit-tests-{gcc,clang}: refactor execute step to use aut…

5e34947

…ogen.sh Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

cmd/snap-confine/apparmor-support: fix implicit fallthrough

f352040

ENOENT case is not expected to fall through. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mvo5 approved these changes Nov 7, 2017

View reviewed changes

mvo5 merged commit aca5f62 into snapcore:master Nov 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/{snap-confine,libsnap-confine-private,snap-shutdown}: cleanup low-level C bits #4153

cmd/{snap-confine,libsnap-confine-private,snap-shutdown}: cleanup low-level C bits #4153

bboozzoo commented Nov 6, 2017

codecov-io commented Nov 6, 2017 •

edited

zyga left a comment

zyga Nov 6, 2017

bboozzoo Nov 6, 2017 •

edited

zyga Nov 6, 2017

bboozzoo Nov 6, 2017

zyga left a comment

zyga Nov 6, 2017

bboozzoo Nov 6, 2017

zyga Nov 6, 2017

bboozzoo Nov 7, 2017

mvo5 left a comment

mvo5 Nov 6, 2017

bboozzoo Nov 6, 2017

mvo5 Nov 6, 2017

mvo5 Nov 6, 2017

mvo5 Nov 6, 2017

bboozzoo commented Nov 6, 2017

cmd/{snap-confine,libsnap-confine-private,snap-shutdown}: cleanup low-level C bits #4153

cmd/{snap-confine,libsnap-confine-private,snap-shutdown}: cleanup low-level C bits #4153

Conversation

bboozzoo commented Nov 6, 2017

codecov-io commented Nov 6, 2017 • edited

Codecov Report

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo Nov 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo commented Nov 6, 2017

codecov-io commented Nov 6, 2017 •

edited

bboozzoo Nov 6, 2017 •

edited