From owner-svn-src-head@freebsd.org Wed Jul 12 10:28:41 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 145F8D93BE1; Wed, 12 Jul 2017 10:28:41 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id B3B9C827CE; Wed, 12 Jul 2017 10:28:40 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 7CCC31A3B3F; Wed, 12 Jul 2017 20:08:43 +1000 (AEST) Date: Wed, 12 Jul 2017 20:08:42 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin 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 In-Reply-To: <201707112155.v6BLtKbZ006618@repo.freebsd.org> Message-ID: <20170712184348.Q1271@besplex.bde.org> References: <201707112155.v6BLtKbZ006618@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=VbSHBBh9 c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=K9qLy5Vgh6matLySN4AA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Jul 2017 10:28:41 -0000 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