cmd: add functions to load/save fstab-like files

[zyga]

This patch adds a mount entry structure that mirrors what is in
struct mntent. It is based on the standard library functions for
parsing and saving fstab files correctly.

The main motivation is that the extra linkage between entries and
(upcoming) utiltiy function make this format preferred for processing.

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

[niemeyer]

Trivial question, but LGTM.

[niemeyer]

Are you sure these fields will never be NULL?

[zyga]

There's no hard specification on that in the manual page but glibc ensures that we get static "" instead of NULL in each of those field, in each case.

[niemeyer]

As discussed, might be worth testing for NULL and strdup("") explicitly if we happen to find it.

[zyga]

Done

[niemeyer]

What an awkward function name.

[zyga]

Indeed, but that's libc :-(

[tyhicks]

There's one NULL check that needs to be added and the mentioned test would be a nice thing to have. The other comments are more for discussion and are not blockers.

[tyhicks]

What's the intent of this memset? I initially assumed that it was to scrub memory but that wouldn't make sense because it is only scrubbing the pointer values (not the strings) and the ints in the sc_mount_entry struct.

[zyga]

It is indeed to scrub the pointers. This is just a habit, so that double-free (if ever is made to happen) becomes harmless.

[tyhicks]

We need to ensure that entryp is not NULL before dereferencing it. That's essential for cleanup functions that are automatically called via the cleanup attribute.

[zyga]

Note that there are two pointers here. Gcc ensures that this is never called with a NULL pointer. What may happen is that we call sc_free_mount_entry_list(NULL) which should be correct (and if it is not, should be made to be correct).

[tyhicks]

In my experience with GCC's cleanup attribute, I have not noticed that GCC ensures that a cleanup function is only called when the pointer is non-NULL. It doesn't say anything in the docs about that:

https://gcc.gnu.org/onlinedocs/gcc-6.3.0/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes

I also noticed that checking for a NULL pointer is something that we do in all of the functions in cmd/libsnap-confine-private/cleanup-funcs.c

[zyga]

If a variable of any type has the cleanup attribute a function is called with a pointer to that variable. That is by definition not NULL. I've added the non-NULL checks there because someone may just use those functions directly. I guess that's a reason enough to add the check here but I wanted to make sure we agree on how the mechanism works.

[tyhicks]

Oh, right. I was confused. Thanks for talking through it with me!

[zyga]

I've added a sanity check for the NULL just in case this is used manually.

[tyhicks]

As mentioned above, we'd see a segfault here due to sc_cleanup_mount_entry_list() not handling a NULL pointer.

[tyhicks]

How about adding a test for sc_load_mount_profile() where you pass it a path to a file that does not exist in order to test this case and keep it from regressing.

[zyga]

+1, will do

[zyga]

Done

[tyhicks]

I feel like it is a mistake to replicate the mntent structure and think the sc_mount_entry struct should look like this:

struct sc_mount_entry {
	struct mntent entry;

	struct sc_mount_entry *next;
};

My reasoning is that you won't need to convert back-and-forth between sc_mount_entry and mntent structs. You'll still need to duplicate the strings in sc_clone_mount_entry_from_mntent() but sc_save_mount_profile() would get smaller because you have a mntent ready to be passed to addmntent().

There may come a time that you want to make use of hasmntopt() and that'll be simple because you have a mntent ready to pass to it.

[zyga]

Ah, that's a nice idea. I will do that, thanks!

[zyga]

Done