cmd/snap-confine: use flags rather than magic bool constants

[zyga]

Using a structute with a boolean is easy to confuse (especially when
looking at bunch of data) so I switched it to a simple enum.

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

[jdstrand]

FWIW, is_bidirectional is very clear to me.

IMHO, declarations should be 'unsigned int' instead of 'unsigned', for clarity. More importantly, the values of the enum are of underlying type 'int', not 'unsigned int'. Furthermore, you are performing bitwise operations to see if they are set when equality would be much clearer. If you were going to add more flags, I suggest changing flags to be 'int' to match the underlying type and to add a comment in the enum that these are bit flags. Eg:

// bit flags for bitwise comparisons
enum {
        SC_MOUNT_BIDIRECTIONAL = 1,
//        SC_MOUNT_FOO = 2,
//        SC_MOUNT_BAR = 4,
//        ...,
};

struct sc_mount {
        const char *path;
        int sc_flags;
};

All that said, I prefer is_bidirectional, is_foo, and is_bar to this. If you are doing this for storage, you could use a bit-field instead and still have similar semantics (ie, equality operations) as with bi_directional.

As is, -1. With the aforementioned changes +0. @tyhicks, thoughts?

[tyhicks]

For code readability, I also think that the move from a well named bool to a bit-field is a step in the wrong direction. If a bit-field were to be used, some helper functions, such as bool sc_mount_is_bidirectional(struct sc_mount *mount);, would improve things.

As for memory footprint, a bunch of bools is obviously worse off than a bit-field but a "bunch of bools" is not a problem that we currently have.

Maybe the commit message doesn't fully convey the reasoning? It says that a structure containing a bool is easy to confuse. Can you expand on what you mean by that?

[niemeyer]

Agree with Tyler and Jamie.

Note that this goes into the exact opposite direction of what we did inside snapd itself.