cmd: add helpers for working with mount/umount commands

[zyga]

This patch adds two new functions that describe mount/umount system call arguments in terms
of option passed to an equivalent mount and umount commands. They are mainly intended for
debugging but can be also used to cut the amount of boilerplate code.

This branch also includes a small new string routine, sc_string_init, because appending is
only sensible given a well-defined initial state.

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

[jdstrand]

filesystemtype is a bit combersome. How about fs_type?

[zyga]

This just follows the manual page but if you prefer I can tweak it.

[jdstrand]

It's fine how it is if that's how you prefer it.

[jdstrand]

You are checking for filesystemtype != NULL here which implies you don't know its contents are a proper C string. strncmp(filesystemtype, "none", 4) may be somewhat better since there aren't any filesystems that start with 'none'.

[zyga]

Nice idea, will do

[jdstrand]

Shouldn't MS_REC only be set if mountflags & MS_REC? Same for with MS_SHARED, MS_SLAVE, MS_PRIVATE and MS_UNBINDABLE in the next sections.

[zyga]

This is done so that we don't redundantly display rec in the generic -o section below. The idea is that since this only makes sense with one of the places that already handle MS_REC we don't display it in the generic section.

[jdstrand]

Since used_special_flags is being used to track what was already added to the buf, even if for the case of MS_REC it doesn't matter, it seems that being precise is better for future-proofing (eg, if the conditional changes, etc) and it makes it a whole lot easier to understand. As such, I'd prefer to see this:

if (mountflags & MS_BIND) {
	if (mountflags & MS_REC) {
		sc_string_append(buf, buf_size, " --rbind");
		used_special_flags |= MS_REC;
	} else {
		sc_string_append(buf, buf_size, " --bind");
	}
	used_special_flags |= MS_BIND;
}

[jdstrand]

This is non-obvious. It appears you are trying to tack on the mount flags to the end of buf based on what is left in buf, and then sc_mount_opt2str() calls sc_string_init() in the middle of the string (probably why you used *buf = 0 in sc_string_init() now that I see this). I think this needs to be rewritten in a manner that is easier to follow. Perhaps create a new string to pass to sc_mount_opt2str() then sc_string_append(buf, buf_size, this_new_string).

[zyga]

Ha, I had that exact code but then I wanted to just use one buffer. I can easily restore that code.

[jdstrand]

Same comment here as for filesystemtype. A strncmp(source, "none", 4) could be better since source shouldn't ever start with 'none'.

[jdstrand]

Same comment here. A strncmp(target, "none", 4) could be better since target shouldn't ever start with 'none'.

[jdstrand]

For consistency style, please use (but see my comments regarding sc_mount_cmd inline):

buf[0] = '\0';

[jdstrand]

Can you adjust this to be: "Initialize a string as empty, ensuring dst is non-NULL and dst_size is > 0"

[jdstrand]

Looks fine with a couple of small requests.

[jdstrand]

How this variable works is a bit subtle. Can we adjust the comments to be:

// Check for some special, dedicated options that aren't represented with mount
// command line arguments by collecting those options that we will display as
// command line arguments in used_special_flags so it may be used to filter out
// these arguments from mount_flags when calling sc_mount_opt2str() later.

Wordsmith that however you like. :)

[zyga]

I think you just convinced me to write my own text editor. It shall be called wordsmith 🗡

[jdstrand]

Shouldn't MS_REC only be set if mountflags & MS_REC? Same for with MS_SHARED, MS_SLAVE, MS_PRIVATE and MS_UNBINDABLE in the next sections.

[zyga]

This is done so that we don't redundantly display rec in the generic -o section below. The idea is that since this only makes sense with one of the places that already handle MS_REC we don't display it in the generic section.

[jdstrand]

Since used_special_flags is being used to track what was already added to the buf, even if for the case of MS_REC it doesn't matter, it seems that being precise is better for future-proofing (eg, if the conditional changes, etc) and it makes it a whole lot easier to understand. As such, I'd prefer to see this:

if (mountflags & MS_BIND) {
	if (mountflags & MS_REC) {
		sc_string_append(buf, buf_size, " --rbind");
		used_special_flags |= MS_REC;
	} else {
		sc_string_append(buf, buf_size, " --bind");
	}
	used_special_flags |= MS_BIND;
}

[jdstrand]

Nitpick: sizeof(opts_buf) for style consistency.

[zyga]

Aww, I really like the bare sizeof :-(

[zyga]

@jdstrand please have a final look

[jdstrand]

s/ so//

[zyga]

Fixed, thank you