Replace mount(2) calls to in-tree filesystems will nmount(2).
AbandonedPublic
Actions

Authored by brooks on Jun 21 2019, 3:15 AM.

Details

Reviewers

kib
markj
jhb

Summary

Most in-tree filesystems support both mount(2) and nmount(2). The
mount(2) API is difficult to translate for non-native ABIs (e.g.,
freebsd32). It is also nearly unused in the base system (amd(8) is the
only consumer).

This change adds translations from the mount(2) API to the nmount(2)
API for known filesystems. Where a translation is unavailable, it
falls back to calling the mount(2) system call.

This is intended to be a step towards allow new ABIs to not support
mount(2) while retaining most functionality.

This patch was developed by (Oleg Derevenetz) with review and minor
changes by me.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25181
Build 23868: arc lint + arc unit

Event Timeline

brooks created this revision.Jun 21 2019, 3:15 AM

Harbormaster completed remote builds in B24971: Diff 58860.Jun 21 2019, 3:15 AM

I'd eventually like to move mount(2) under COMPATXX, but that's probably a long road. I've talked to one storage vendor who hasn't migrated their internal filesystem to nmount yet which suggests there are more.

There is a lot of style(9) violations, and I stopped listing at some point. Each thing I noted is recurrent.

From the description, it is not clear to me why do we need to add this to libc. Is this to allow to run amd(8) ? If yes, it is going out, I hope. Otherwise, the only use of mount(2) is to provide compat for old userspace (old enough to predate versioned libc), which cannot take advantage of the new libc in any case. I think that some vendor that still uses mount(2) internally have to fix the use on his own.

What I dislike from this commit is that the translations from mount(2) to nmount(2) occur in two places now.

lib/libc/sys/mount.c
57	The '{' should be on the previous line.
67	The use of spaces is wrong.
77	The number of blank lines in this function is same as non-blank lines. I think none of the blank lines, except after locals declarations, are needed.
78	Why 4 ?
102	Why 2 ? IOV, add comment that we are adding name and value.
195	We do not put tab after type in local decl.
200	{} are excessive.
230	{}
241	Again tabs/spaces before '='.
310	() are excessive.
421	static ?
444	I think using nitems() is better than NULL-terminate static array.
463	Why do you use _nmount there but __sys_mount later ?

If we can actually get rid of amd that would be sufficient for my local uses, but I doubt it's the only consumer.

I've been unable to find a good way to audit ports for mount() use so I don't know how many consumers there are. I suppose I could request and exp-run with the prototype and symbols removed to see. To the extent that there are consumers, this helps with freebsd32 (or in a CHERI world freebsd64) because there are fewer structs that require translation.

I'm not especially concerned about duplicating this logic. It's not something we'll need to maintain since we're not going to be adding more mount() filesystems to the tree.

Explain magic numbers in comments.
Make supported_fs static.
Use nitems() rather then a NULL terminated array.

Harbormaster completed remote builds in B25111: Diff 59165.Jun 28 2019, 6:48 PM

I've addressed the main technical concerns (other then which syscall implementation to call). If we agree to move forward with this patch I can address style(9), but don't want to spend the time unless it's going to land in FreeBSD.

lib/libc/sys/mount.c
463	In the case of `_nmount`, my (possibly misguided) understanding is that we generally call sys calls by their `_` version inside libc. This was originally `nmount`. The __sys_mount call is to call the syscall directly without any interposition.

In D20716#450173, @brooks wrote:

I've addressed the main technical concerns (other then which syscall implementation to call). If we agree to move forward with this patch I can address style(9), but don't want to spend the time unless it's going to land in FreeBSD.

It is strange to have such non-trivial emulation both in libc and in kernel. I would not object strongly against adding this code, but my personal opinion is that we better get rid of amd instead.