Hi Thanks for the comments.
2018-03-27 0:57 GMT+09:00 Martin Pieuchot <m...@openbsd.org>: > Hello, > > On 25/03/18(Sun) 03:53, Tomohiro Kusumi wrote: >> This is the initial patch submission of autofs filesystem support for >> OpenBSD, which was originally written for FreeBSD and later ported to >> DragonFlyBSD and NetBSD. >> >> Autofs is a generic filesystem automounter filesystem, not limited to >> mounting NFS. > > Does that mean amd(8) becomes obsolete? If yes, how does an autofs Yes, basically. FreeBSD still has amd(8), but DragonFly removed it. They can technically coexist though. > setup can replace an amd(8) one? I don't know too much about amd(8), but there is no compatibility. The migration is about replacing amd(8) setup with /etc/auto_master which is not all that difficult. The FreeBSD autofs is aimed to be compatible with Solaris autofs, and https://people.freebsd.org/~trasz/autofs.pdf explains a bit on this. > >> mounting NFS. This OpenBSD port actually started off with the NetBSD >> code rather than FreeBSD though all versions are quite similar except >> for the autofs filesystem code in kernel space. While I currently >> maintain it as an out-of-tree filesystem on GitHub, I'm aiming for >> this to get merged by OpentBSD. > > That's really nice. > >> The initial patch is available from below in three formats. The patch >> applies to the current src as of March 24 (and builds on amd64). >> >> diff -aNur >> https://leaf.dragonflybsd.org/~tkusumi/diff/openbsd/OpenBSD-autofs-v1.patch >> git format-patch >> https://leaf.dragonflybsd.org/~tkusumi/diff/openbsd/0001-OpenBSD-autofs-v1.patch >> GitHub >> https://github.com/kusumi/src/tree/autofs-20180325 > > Please submit the diff inline next time. That allow direct review > without relying on other tools than a mail client. Sure. I didn't inline the patch as it was a big chunk of text. > > I have multiple comments, some of them can be addressed in tree: > > - It's MI code so /dev/autofs should be present on all archs. OK. I'll find MI configuration and move autofs from amd64. > > - Since it's MI code it should be tested on more than amd64, sparc64 > and a 32bit/gcc arch would be great. I only have x86. I can give a try for IA32 on a virtual machine at some point. > > - Wouldn't it be better to put the configuration file examples to > /etc/examples/autofs? /etc/autofs/* aren't examples, but rather features, meaning users are not going to copy these and edit. They just use these scripts, or they add new scripts for new features that they want. In fact, FreeBSD autofs has evolved that way. What users are going to edit is /etc/auto_master and user defined scripts refered to by /etc/auto_master. > > - Could you please reduce the number of sysctl buttons, or even better > remove them completely? `autofs_debug_tunable' is certainly not > needed. I can't say for the others but I'd prefer if we could have > values that work for everybody. All these come from FreeBSD, rather than my preference. I'm fine with removing these entirely if they bother OpenBSD. The default values shouldn't have problems for most cases. > > - You can update the OpenBSD release for the man pages to 6.4 OK. > > - The documentation for EVFILT_FS is misleading. I see that it comes > from DragonFly however using "Currently" implies that something else > will be supported in the future. Is it your plan? How does this > implementation differs from Darwin/Mac OS X? FreeBSD has other events that trigger EVFILT_FS that are not needed by autofs. That's what "currently" implies, but I can drop that. I'm not going to implement the rest anyway. > > - Since file(1) on OpenBSD is privilege separated and pledge(2)'d do we > really need fstyp(8)? What does it buy us? Nothing really. I can make this a part of usr.sbin/autofs rather than as a stand alone command if this bothers OpenBSD. While this command is a generic command, some features are specifically needed by autofs. fstyp(8) also supports multi-volumes filesystem such as HAMMER. By the way, I can drop exfat.c (a copy from below) if OpenBSD does not prefer to have exFAT related code. https://github.com/freebsd/freebsd/blob/master/usr.sbin/fstyp/exfat.c > > - Why do we need two daemons? Can we merge the functionalities of > autounmountd(8) into automountd(8)? automountd(8) blocks at ioctl(2) via /dev/autofs waiting for mount requests. autounmountd(8) blocks waiting for kqueue(2) events. I recommend to have these separated for simplicity. > > - It would be nice if our common log.c would be used instead of adding > a new one. I'll look into log.c. Didn't know such thing exists. > > - Could you change the RB_* macros to RBT_* macros in the kernel code? OK. > > - Why didn't you implemented the set/restore sigmask? Did you encounter > any problem? I couldn't find a way to do this. I think I saw NFS doing something with signal mask though not the exact thing I was looking for. This isn't a must have feature, but should be implemented. In fact, OpenBSD autofs is the only autofs missing this feature. > > - Do we really need a custom task queue? Can't we use the system > task queue for sending timeouts? OK. Didn't know it exists. I'll try to find that. > > - Instead of doing atomic_add_int_nv(..., -1) you can use > atomic_dec_int_nv(). OK. > > - APRINTF() is not used and can go, __inline -> inline, I'd keep kernel > headers in autofs_*.c rather than "autofs.h". I'll drop APRINTF() at the final moment if this ever gets merged. > > - I fear there's a missing reference that can lead to a use-after free. > You're giving a request reference to your timeout routing without > accounting it. So when timeout_del() is called the task could be > spinning on the lock. > > - Could you add VLOCKSWORK to v_flag if VFSLCKDEBUG is defined? It > might help us find possible locking issues. OK. > > - Did you try running the code with a WITNESS kernel? Are you sure No. Didn't know about "WITNESS" kernel. > there's no locking ordering issues between `am_lock' and `an_vn_lock'? > >> [...] >> 3. The "-media" map is currently unsupported due to >> etc/autofs/special_media not being functional. The same script in >> FreeBSD uses GEOM sysctl, whereas there seems to be no easy way to >> achieve the same result on OpenBSD. See the bottom part of >> etc/autofs/special_media for details. I'd apply if this is doable >> on OpenBSD, which is to list all possible block devices and >> partitions for fstyp to check filesystem type. > > That's a crucial point. If that's for removable media, how does the > deamon know when a USB key for example, is inserted? Can't you use > /dev/hotplug and disklabel(8)? The requirement here is to get the list of block devices rather than catching hardware events, but if the kernel doesn't provide a way to retrieve such information, it boils down to low level things like hotplug, searching dmesg, etc. But that too isn't a complete solution because block device could be a partition, or some form of ramdisk, or regular file backed device, etc. I recommend to leave this for now, as this is more than just autofs. To be exact, DragonFly (and I think NetBSD too) can handle this if the target device is the entire physical device via sysctl kern.disks, but I have commented it out since it's a limited solution. An example of good solution is sysfs in Linux, where any sort of device registered to kernel data structures are exposed to userspace via virtual filesystem, which makes this sort of thing much easier.
