Differential D523

Autofs
AbandonedPublic
Actions

Authored by trasz on Aug 2 2014, 2:10 PM.

Details

Reviewers

kib

Group Reviewers

Doc Committers

Summary

Add new automounter implementation, compatible with Sun automounter, and with proper kernel support.

Sponsored by: The FreeBSD Foundation
MFC After: 2 weeks

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

wblock added inline comments.Aug 4 2014, 5:57 PM

usr.sbin/automountd/auto_master.5
49	The commas in this sentence are not really needed: "...consists of lines with two or three entries separated by whitespace and terminated by a newline character:"
51	This should probably be marked up: .Pa mountpoint Pa map_name Op Ar -options
54	.Pa mountpoint is either a fully specified path, or .Li /- .
55	When .Pa mountpoint is a full path, .Pa map_name must reference an indirect map. (but what is that? "indirect map" has not been defined) Otherwise, .Pa map_name must reference a direct map. ("reference" is not very clear. "Point to"? "Be the name of a map defined in..."? "See .Sx "MAP SYNTAX" below."?)
58	.Pa map_name specifies the map to use.
59	If .Pa map_name begins with .Li - , it indicates a special map. See .Sx "MAP SYNTAX" below.
60	If .Pa map_name is not a fully specified path .Pq it does not start with Li / ,
62	This is not clear. I think it means this, but please verify: will search for a file with that name in .Pa /etc . Otherwise, it will use the path as given.
64	If the file indicated by .Pa map_name is executable,
66	s/it's/it is/ (do not use contractions) See .Sx "MAP SYNTAX" below.
67	"It" is general and sometimes difficult to identify. How about: Otherwise, the file is opened and the contents parsed.
69	.Pa -options is an optional field that starts with .Li - and can contain generic filesystem mount options. (Note: "generic" is often confusing to those with English as a second language.) (Maybe add a "See .Xr mount 8 ." here? Although this probably refers to specific filesystem mount options.)
72	Avoid saying "the following" when possible, preferring "This" or "These". These intermixed examples should probably be separated out into a standard Examples section.
79	Modify as above.
81	.Pa key Oo Ar -options Oc Oo Ar mountpoint Oo -options Oc Oc Ar location Op ... (untested, that should be close to get the same display) (I'm guessing that "key" is a path.)
84	s/it's/it is/ Reads a little better as "In most cases, it can be simplified to:"
86	.Pa key Op Ar -options Ar location (untested)
89	.Pa key is the path component used by
94	The .Ar -options field, if present, must begin with .Li - .
97	The aside makes this a bit confusing. "When mounting the filesystem, options supplied to .Nm and options specified in the map entry file are combined and used."
98	The special option .Li fstype is used to specify filesystem type.
99	It is not passed to the mount program as an option. Instead,
100	I'm not sure of the markup, but this should be something like it is passed as an argument to .Cm mount -t
102	Use .Pa mountpoint
105	Use .Ar location
106	"To pass a location that begins with .Li / , prefix it with a colon. For example, .Li :/dev/cd0 .
109	This example, when used with the
111	.Li 192.168.1.1:/share/example/x
112	will be mounted on .Pa /example/x/ when any process attempts to access that mountpoint, with .Li intr and .Li nfsv4 mount options.
115	.Li x -intr,nfsv4 192.168.1.1:/share/example/x
118	Automatically mount the CD drive on access:
120	.Li cd -intr,fstype=cd9660 :/dev/cd0
123	Special maps have names beginning with .Li - .
127	This should probably be .It Li -hosts
129	It is traditionally mounted on .Pa /net .
131	.Pa /net/nfs-server-ip/share-name/ directory , without the need for any
133	Likewise, I think .It Li -null
136	A little redundancy can be removed: "This map prevents .Xr automountd 8 from mounting anything on the mountpoint."
139	If the map file specified in .Pa auto_master has the execute bit set,
141	will execute it and parse the standard output instead of parsing
142	the file contents.
150	by entries with .Li /- as the mountpoint, and must contain only fully qualified
152	For indirect maps, the final mount point is determined by concatenating the
154	mountpoint with the map entry key and the optional map entry mountpoint.
155	For direct maps, the final mount point is determined by concatenating the map entry key with the optional map entry mountpoint.
157	The example above can be rewritten using a direct map, by placing this in

Comments on automount.8.

usr.sbin/automountd/automount.8
52	Try to avoid "the following". "These" will work here.
58	Yes, .Fl L But I don't understand this sentence. "Defining variables is only useful when -L is used with .Nm"?
60	Avoid semicolons: Do not mount or unmount anything. Instead, parse
62	Remove "and".
67	.Fl u .
69	s/with ones/with the ones/
70	Again, this sentence is difficult to decipher. Try to avoid "Note", the whole man page really is prefixed with that. "In case of" and "in the case of" are not very meaningful and can usually be left out.
72	.Fl L .
76	Yes, just .Xr automountd 8 .
78	This is another unclear sentence. .Xr autofs 5 mounts are not unmounted.
79	To unmount all .Xr autofs 5 mounts, use .Cm "umount -At autofs" . Or maybe use .Bd -literal -offset indent umount -At autofs .Ed

Comments on automountd.8.

usr.sbin/automountd/automountd.8
47	.Xr autofs 5
48	The meaning of "appropriate" is not clear.
51	On startup, .Nm forks into the background and waits for kernel requests.
54	s/parses/parses the/
56	This sentence is very long, and really could stand to be split up: When a request is received, .Nm forks a child process. The child process parses the appropriate map and mounts filesystems accordingly. Then it signals the kernel to release blocked processes that were waiting for the mount.
67	This is another sentence that needs to be split up. Also, avoid indefinite articles when possible (this option). Also, avoid passive voice. Change "will create" to "creates". Without .Fl i , .Nm creates all the subdirectories it can. Users may not realize that the wildcard map entry makes it possible to access directories that have not yet been created.
68	If this flag takes an argument, it should be shown: .It Fl m Ar n
69	Limit the number of forked .Nm processes, and thus the number
73	Debug mode: increase verbosity and do not daemonize.
74	Again, an argument: .It Fl o Ar mountoptions Use the same name as in the synopsis.
79	Why not just tell the user Options specified here will be overridden by options entered in maps or .Xr auto_master 5 .

Comments on autounmountd.8.

usr.sbin/automountd/autounmountd.8
45	.Xr automountd 8 .
46	On startup, .Nm retrieves a list of filesystems that have the .Li automounted (there might be better markup to use for mount options, not sure)
48	The list of filesystems is updated each time a filesystem is mounted or unmounted.
49	After a specified time passes,
53	s/The following/These/
56	Debug mode: increase verbosity and do not daemonize.
60	Number of seconds to wait before trying to unmount an expired filesystem after a previous attempt failed, possibly due to the filesystem being busy.
61	The default value is 600, or ten minutes.
64	The default value is 600, or ten minutes.

emaste added a subscriber: emaste.Aug 5 2014, 1:54 PM

emaste added inline comments.Aug 5 2014, 2:04 PM

etc/autofs/include_ldap
2	should we have a $FreeBSD$ tag?
10	failure handling if no ldap port installed?
share/man/man5/autofs.5
56	the process triggering the mount request. Not sure how to write it in a non-awkward way

kib added inline comments.Aug 7 2014, 5:14 AM

sys/fs/autofs/autofs.c
164	Style. No initialization in declarator. This is systematic issue.
171	Style. Use C comments. Or just remove this debugging stuff, or convert it to CTR.
188	Should there be some assertion which guarantees stability of the an_parent walk ?
190	Style. Line wrap.
196	Why is this assignment needed ?
502	Why do you try to prevent multiple opens ? This is absolutely ineffective WRT preventing parallel accesses to the device, and I do not see why do you need it at all.
sys/fs/autofs/autofs.h
62	This is traditionally spelled ASSERT_UNLOCKED()
sys/fs/autofs/autofs_vfsops.c
79	What is the point of having the private per-mp pointer to the global device data ? It looks like useless indirection.
sys/fs/autofs/autofs_vnops.c
85	== false is awful, really. Use !cond.
150	This is racy, since vp lock is dropped. What does prevent a mount over the vp ?
183	Why do you need this vfs_busy() call ? While covered vnode is locked, unmount cannot proceed .
213	And there vfs_busy() is needed. Consider using vn_vget_ino() instead of hand-rolling it.

emaste added inline comments.Aug 7 2014, 1:53 PM

sys/fs/autofs/autofs.c
171	Yes, no point in leaving in commented-out debug. You could also have a compile-time option to turn it into an empty #define AUTOFS_DEBUG(...) and leave them in?

All the issues above should be fixed before it was actually committed; I just never uploaded an updated diff.

sys/fs/autofs/autofs.c
502	That's because we can't ever block automountd(8) and its descendants, and we use session ID to determine that - we store session id of the process that opened the device, and then compare it with session ids of triggering processes. Having the device open by several different processes, with different session ids, would mess that up and result in deadlock, due to automountd(8) getting blocked.