vfs mount: Consistently use ENODEV internally for an invalid fstype
ClosedPublic
Actions

Authored by jhb on Oct 23 2023, 3:47 AM.

Details

Reviewers

markj
pstef

Commits

rGb636e35bb724: vfs mount: Consistently use ENODEV internally for an invalid fstype
rG3eed4803f943: vfs mount: Consistently use ENODEV internally for an invalid fstype

Summary

Change vfs_byname_kld to always return an error value of ENODEV to
indicate an unsupported fstype leaving ENOENT to indicate errors such
as a missing mount point or invalid path. This allows nmount(2) to
better distinguish these cases and avoid treating a missing device
node as an invalid fstype after commit 6e8272f317b8.

While here, change mount(2) to return EINVAL instead of ENODEV for an
invalid fstype to match nmount(2).

PR: 274600

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jhb created this revision.Oct 23 2023, 3:47 AM

Herald added a subscriber: imp. · View Herald TranscriptOct 23 2023, 3:47 AM

jhb requested review of this revision.Oct 23 2023, 3:47 AM

Harbormaster completed remote builds in B54125: Diff 129228.Oct 23 2023, 3:48 AM

markj added inline comments.Oct 23 2023, 1:46 PM

sys/kern/vfs_init.c
153	Do we want to translate all errors to ENODEV, or just ENOENT?

jhb added inline comments.Oct 23 2023, 4:44 PM

sys/kern/vfs_init.c
153	I think all. The call in ZFS ignores *error and always returns ENODEV already. I think from the caller's perspective it's still true after a failed kldload that vfs_byname() would return NULL and that's the main error we want to communicate here.

What are the user-facing errors in various cases after this?

In D42327#966443, @pstef wrote:

What are the user-facing errors in various cases after this?

Using an invalid path like /dev/foobar now returns ENOENT again as it did before 6e8272f317b8. Trying to use an invalid fstype that fails to kldload still returns EINVAL from nmount(2) (a change made in 6e8272f317b8) and mount(2) now returns EINVAL for that case instead of ENOENT. Before 6e8272f317b8 the invalid fstype case probably returned ENOENT but might have possibly returned ENODEV. The invalid fstype case is not documented in the mount(2) manpage, but the ENOENT error is, so for the documented ENOENT case this commit fixes a regression in 6e8272f317b8.

Thanks for the information and apologies for the mess.
For me the case where the filesystem name was wrong and mount complained about file not existing was more common and more annoying since it was easy to see that the designated mount point clearly existed.

I'd vote for pushing this into the upcoming release.

This revision is now accepted and ready to land.Oct 25 2023, 4:01 PM

markj accepted this revision.Nov 2 2023, 3:34 PM

Closed by commit rG3eed4803f943: vfs mount: Consistently use ENODEV internally for an invalid fstype (authored by jhb). · Explain WhyNov 18 2023, 7:10 PM

This revision was automatically updated to reflect the committed changes.

jhb added a commit: rG3eed4803f943: vfs mount: Consistently use ENODEV internally for an invalid fstype.

jhb added a commit: rGb636e35bb724: vfs mount: Consistently use ENODEV internally for an invalid fstype.Jan 3 2024, 8:33 PM

@jhb do you think the following is related?

root@precision:~ # gpart show da0
=>       63  500118129  da0  MBR  (238G)
         63       1985       - free -  (993K)
       2048   67108864    1  fat32lba  [active]  (32G)
   67110912  433007280       - free -  (206G)

root@precision:~ # mount /dev/da0s1 /tmp/empty
mount: /dev/da0s1: No such file or directory
root@precision:~ # mount -t msdosfs /dev/da0s1 /tmp/empty
root@precision:~ #

Herald added a subscriber: olce. · View Herald TranscriptMar 9 2024, 9:32 PM

Ah, I see my error. For some reason I had convinced myself that this fixed has been MFC'd, but it hasn't. I'm running stable/13. Sorry for the noise.

zlei mentioned this in D44633: ng_socket: Treat EEXIST from kern_kldload() as success in case there is a race condition.Apr 4 2024, 3:35 PM