Date: Wed, 12 Jul 2017 20:08:42 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r320900 - in head/sys: fs/cd9660 fs/ext2fs fs/fifofs fs/msdosfs fs/nandfs fs/nfsclient fs/smbfs fs/tmpfs ufs/ufs Message-ID: <20170712184348.Q1271@besplex.bde.org> In-Reply-To: <201707112155.v6BLtKbZ006618@repo.freebsd.org> References: <201707112155.v6BLtKbZ006618@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 11 Jul 2017, John Baldwin wrote:
> Log:
> Consistently use vop_stdpathconf() for default pathconf values.
>
> Update filesystems not currently using vop_stdpathconf() in pathconf
> VOPs to use vop_stdpathconf() for any configuration variables that do
> not have filesystem-specific values. vop_stdpathconf() is used for
> variables that have system-wide settings as well as providing default
> values for some values based on system limits. Filesystems can still
> explicitly override individual settings.
This is sort of backwards. Not many settings are the same for most
file systems, and many that did were wrong. Now more are wrong.
> Modified: head/sys/fs/cd9660/cd9660_vnops.c
> ==============================================================================
> --- head/sys/fs/cd9660/cd9660_vnops.c Tue Jul 11 21:52:11 2017 (r320899)
> +++ head/sys/fs/cd9660/cd9660_vnops.c Tue Jul 11 21:55:20 2017 (r320900)
> @@ -792,20 +792,11 @@ cd9660_pathconf(ap)
> else
> *ap->a_retval = 37;
> return (0);
> - case _PC_PATH_MAX:
> - *ap->a_retval = PATH_MAX;
> - return (0);
PATH_MAX is one of the few that is system-wide.
> - case _PC_PIPE_BUF:
> - *ap->a_retval = PIPE_BUF;
> - return (0);
PIPE_BUF isn't system wide. cd9660 does support fifos, but setting it
for files that aren't fifos or directories is bogus. POSIX allows such
bogus settings, and needs complicated wording to describe this. E.g.,
PIPE_BUF for a directory is not completely bogus and it means that the
value for the directory applies to all fifos in the directory and is
ignored for all non-fifos in the directory. It is unclear if PIPE_BUF
must be set for a directory if there is any fifo in the directory.
FreeBSD's man pages give no details about this.
> - case _PC_CHOWN_RESTRICTED:
> - *ap->a_retval = 1;
> - return (0);
This is system-wide.
> case _PC_NO_TRUNC:
> *ap->a_retval = 1;
> return (0);
This is almost system-wide.
> default:
> - return (EINVAL);
> + return (vop_stdpathconf(ap));
This is fail-unsafe. It gives bogus settings like PIPE_BUF for all cases.
Case statements in file systems need to be even larger to kill defaults,
and the code becomes harder to read since it is harder to see the final
values.
vop_stdpathconf() has defaults for:
- ASYNC_IO. Is this system-wide (all in vfs?)
- NAME_MAX. This is very far from system-wide
- PATH_MAX. This is system-wide.
- LINK_MAX. This is very far from system-wide, especially after expansion
of nlink_t. Most file systems get this wrong. LINK_MAX is bogusly
defined to be the constant 32767, but the value is very variable.
zfs_pathconf() returns INT_MAX and zfs uses something like int32_t
internally, but zfs_getattr() clamps to LINK_MAX. ext2fs limits to
32000 for ext2 and 65000 for ext4 internally but ext2_pathconf()
only returns this limit in some cases -- other cases return INT_MAX.
- MAX_CANON. This is garbage except for terminals (and directories).
Its value is also wrong.
- MAX_INPUT. Like MAX_CANON.
- PIPE_BUF. Like MAX_CANON, for fifos instead of terminals.
- CHOWN_RESTRICTED. Wrong for file systems that don't support ownerships
or changing them. Might depend on mount options.
- VDISABLE. Like MAX_CANON.
This is easy to improve:
- NAME_MAX, LINK_MAX: move to individual fs's
- MAX_CANON, MAX_INPUT, VDISABLE: move to devfs_pathconf().
- PIPE_BUF: move to fifo_pathconf()? What about directories?
> }
> /* NOTREACHED */
Bogus comment. Even lint should understand that default prevents reaching
this.
> Modified: head/sys/fs/fifofs/fifo_vnops.c
> ==============================================================================
> --- head/sys/fs/fifofs/fifo_vnops.c Tue Jul 11 21:52:11 2017 (r320899)
> +++ head/sys/fs/fifofs/fifo_vnops.c Tue Jul 11 21:55:20 2017 (r320900)
> @@ -337,34 +336,6 @@ fifo_print(ap)
> fifo_printinfo(ap->a_vp);
> printf("\n");
> return (0);
> -}
> -
> -/*
> - * Return POSIX pathconf information applicable to fifo's.
> - */
> -static int
> -fifo_pathconf(ap)
> - struct vop_pathconf_args /* {
> - struct vnode *a_vp;
> - int a_name;
> - int *a_retval;
> - } */ *ap;
> -{
> -
> - switch (ap->a_name) {
> - case _PC_LINK_MAX:
> - *ap->a_retval = LINK_MAX;
> - return (0);
> - case _PC_PIPE_BUF:
> - *ap->a_retval = PIPE_BUF;
> - return (0);
> - case _PC_CHOWN_RESTRICTED:
> - *ap->a_retval = 1;
> - return (0);
> - default:
> - return (EINVAL);
> - }
> - /* NOTREACHED */
> }
This was small and hopefully correct. Not many settings apply to fifos.
It seems a but too small to be correct -- why would LINK_MAX apply but
not PATH_MAX? Using the default, a pathconf which wants to set only 3
value would need about 15 cases to kill all the defaults.
> Modified: head/sys/fs/msdosfs/msdosfs_vnops.c
> ==============================================================================
> --- head/sys/fs/msdosfs/msdosfs_vnops.c Tue Jul 11 21:52:11 2017 (r320899)
> +++ head/sys/fs/msdosfs/msdosfs_vnops.c Tue Jul 11 21:55:20 2017 (r320900)
> @@ -1875,17 +1875,11 @@ msdosfs_pathconf(struct vop_pathconf_args *ap)
> case _PC_NAME_MAX:
> *ap->a_retval = pmp->pm_flags & MSDOSFSMNT_LONGNAME ? WIN_MAXLEN : 12;
> return (0);
> - case _PC_PATH_MAX:
> - *ap->a_retval = PATH_MAX;
> - return (0);
> - case _PC_CHOWN_RESTRICTED:
> - *ap->a_retval = 1;
> - return (0);
> case _PC_NO_TRUNC:
> *ap->a_retval = 0;
> return (0);
> default:
> - return (EINVAL);
> + return (vop_stdpathconf(ap));
msdosfs doesn't support pipes. It used to indicate this by not having
_PC_PIPE_MAX. Now it gets this via pollution in vop_stdpathconf(ap).
msdosfs also has unusual handling for LINK_MAX, NAME_MAX and NO_TRUNC.
It still has this, with buggy limits in all cases:
- msdosfs doesn't support links. It returns 1 for LINK_MAX instead of -1
(unsupported)
- NAME_MAX doesn't really apply to shortnames. 12 is returned instead
of the 2 limits 8 and 3.
- msdosfs truncates only for shortnames, so NO_TRUNC = 0 is wrong for
!shortnames.
Almost all file systems except msdosfs return 1 for NO_TRUNC, but this
is not defaulted.
> Modified: head/sys/ufs/ufs/ufs_vnops.c
> ==============================================================================
> --- head/sys/ufs/ufs/ufs_vnops.c Tue Jul 11 21:52:11 2017 (r320899)
> +++ head/sys/ufs/ufs/ufs_vnops.c Tue Jul 11 21:55:20 2017 (r320900)
> @@ -2442,21 +2442,9 @@ ufs_pathconf(ap)
>
> error = 0;
> switch (ap->a_name) {
> - case _PC_LINK_MAX:
> - *ap->a_retval = LINK_MAX;
> - break;
> case _PC_NAME_MAX:
> *ap->a_retval = UFS_MAXNAMLEN;
> break;
LINK_MAX is basically ffs's limit.
NAME_MAX _is_ ffs's limit, but it is still defaulted.
I expect that some of the other defaulted limits will become more variable
(so unsuitable as defaults). LINK_MAX is almost there.
> @@ -2505,11 +2493,6 @@ ufs_pathconf(ap)
> case _PC_MIN_HOLE_SIZE:
> *ap->a_retval = ap->a_vp->v_mount->mnt_stat.f_iosize;
> break;
> - case _PC_ASYNC_IO:
> - /* _PC_ASYNC_IO should have been handled by upper layers. */
> - KASSERT(0, ("_PC_ASYNC_IO should not get here"));
> - error = EINVAL;
> - break;
> case _PC_PRIO_IO:
> *ap->a_retval = 0;
> break;
But some of the newer limits like are in vfs so should have been defaulted?
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170712184348.Q1271>