Date: Fri, 28 Jun 2019 11:22:47 +0200 From: Conrad Meyer <cem@freebsd.org> To: Scott Long <scottl@samsco.org> Cc: Alan Somers <asomers@freebsd.org>, Konstantin Belousov <kostikbel@gmail.com>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r349231 - in head/sys: kern sys ufs/ufs Message-ID: <CAG6CVpW_nD-B%2BKCb25Pik7SYWntFnsh9huVuHEzspXG2bQCVfA@mail.gmail.com> In-Reply-To: <0988866D-918B-4800-ADE7-938E1EBD729C@samsco.org> References: <201906201413.x5KEDB5u010923@repo.freebsd.org> <20190627191843.GQ8697@kib.kiev.ua> <CAOtMX2hO3ed=GevyDmzh8VOVT3Zw5yYNKpCBENrWSm_w2nuM3g@mail.gmail.com> <20190627210940.GR8697@kib.kiev.ua> <0988866D-918B-4800-ADE7-938E1EBD729C@samsco.org>
next in thread | previous in thread | raw e-mail | index | archive | help
To this end, __result_use_check can help ensure no unchecked callers are missed when panics are downgraded to errors. (In OneFS, the macro has the imo more memorable name =E2=80=9C__must_check=E2=80=9D. Cheers, Conrad On Thu, Jun 27, 2019 at 23:48 Scott Long <scottl@samsco.org> wrote: > > > > On Jun 27, 2019, at 3:09 PM, Konstantin Belousov <kostikbel@gmail.com> > wrote: > > > > On Thu, Jun 27, 2019 at 02:01:11PM -0600, Alan Somers wrote: > >> On Thu, Jun 27, 2019 at 1:18 PM Konstantin Belousov < > kostikbel@gmail.com> wrote: > >>> > >>> On Thu, Jun 20, 2019 at 02:13:11PM +0000, Alan Somers wrote: > >>>> Author: asomers > >>>> Date: Thu Jun 20 14:13:10 2019 > >>>> New Revision: 349231 > >>>> URL: https://svnweb.freebsd.org/changeset/base/349231 > >>>> > >>>> Log: > >>>> Add FIOBMAP2 ioctl > >>> > >>>> > >>>> This ioctl exposes VOP_BMAP information to userland. It can be used > by > >>>> programs like fragmentation analyzers and optimized cp > implementations. But > >>>> I'm using it to test fusefs's VOP_BMAP implementation. The "2" in > the name > >>>> distinguishes it from the similar but incompatible FIBMAP ioctls in > NetBSD > >>>> and Linux. FIOBMAP2 differs from FIBMAP in that it uses a 64-bit > block > >>>> number instead of 32-bit, and it also returns runp and runb. > >>>> > >>>> Reviewed by: mckusick > >>>> MFC after: 2 weeks > >>>> Sponsored by: The FreeBSD Foundation > >>>> Differential Revision: https://reviews.freebsd.org/D20705 > >>>> > >>>> Modified: > >>>> head/sys/kern/vfs_vnops.c > >>>> head/sys/sys/filio.h > >>>> head/sys/ufs/ufs/ufs_bmap.c > >>>> > >>>> Modified: head/sys/kern/vfs_vnops.c > >>>> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > >>>> --- head/sys/kern/vfs_vnops.c Thu Jun 20 13:59:46 2019 > (r349230) > >>>> +++ head/sys/kern/vfs_vnops.c Thu Jun 20 14:13:10 2019 > (r349231) > >>>> @@ -1458,6 +1458,25 @@ vn_stat(struct vnode *vp, struct stat *sb, > struct ucre > >>>> return (0); > >>>> } > >>>> > >>>> +/* generic FIOBMAP2 implementation */ > >>>> +static int > >>>> +vn_ioc_bmap2(struct file *fp, struct fiobmap2_arg *arg, struct ucre= d > *cred) > >>> I do not like the fact that internal kernel function takes the > >>> user-visible structure which results in the mix of ABI and KBI. > >>> Traditionally we pass explicit arguments to kern_XXX, vn_XXX and > similar > >>> internal implementations. > >> > >> Ok. It will increase the number of function arguments, which is ugly, > >> but I can do it if you prefer. > >> > >>> > >>>> +{ > >>>> + struct vnode *vp =3D fp->f_vnode; > >>> Why do you pass fp to the function that > >>> 1. Has vn_ namespace, i.e. operating on vnode. > >>> 2. Only needs vnode to operate on. > >>> Please change the argument from fp to vp. You would need to pass > f_cred > >>> as additional argument, or move mac check to vn_ioctl (I think this i= s > >>> a better approach). > >> > >> Ok. > >> > >>> > >>>> + daddr_t lbn =3D arg->bn; > >>> Style: initialization in declaration. > >>> > >>>> + int error; > >>>> + > >>>> + vn_lock(vp, LK_SHARED | LK_RETRY); > >>>> +#ifdef MAC > >>>> + error =3D mac_vnode_check_read(cred, fp->f_cred, vp); > >>>> + if (error =3D=3D 0) > >>>> +#endif > >>>> + error =3D VOP_BMAP(vp, lbn, NULL, &arg->bn, &arg->runp= , > >>>> + &arg->runb); > >>> Wrong indent for continuation line. > >>> > >>>> + VOP_UNLOCK(vp, 0); > >>>> + return (error); > >>>> +} > >>>> + > >>>> /* > >>>> * File table vnode ioctl routine. > >>>> */ > >>>> @@ -1481,6 +1500,9 @@ vn_ioctl(struct file *fp, u_long com, void > *data, stru > >>>> if (error =3D=3D 0) > >>>> *(int *)data =3D vattr.va_size - > fp->f_offset; > >>>> return (error); > >>>> + case FIOBMAP2: > >>>> + return (vn_ioc_bmap2(fp, (struct > fiobmap2_arg*)data, > >>> Need space between fiobmap2_arg and '*'. > >>>> + active_cred)); > >>> Wrong indent. > >>> > >>>> case FIONBIO: > >>>> case FIOASYNC: > >>>> return (0); > >>>> > >>>> Modified: head/sys/sys/filio.h > >>>> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > >>>> --- head/sys/sys/filio.h Thu Jun 20 13:59:46 2019 > (r349230) > >>>> +++ head/sys/sys/filio.h Thu Jun 20 14:13:10 2019 > (r349231) > >>>> @@ -62,6 +62,13 @@ struct fiodgname_arg { > >>>> /* Handle lseek SEEK_DATA and SEEK_HOLE for holey file knowledge. */ > >>>> #define FIOSEEKDATA _IOWR('f', 97, off_t) /* SEEK_DATA */ > >>>> #define FIOSEEKHOLE _IOWR('f', 98, off_t) /* SEEK_HOLE */ > >>>> +struct fiobmap2_arg { > >>>> + int64_t bn; > >>>> + int runp; > >>>> + int runb; > >>>> +}; > >>> This structure has different layout for LP64 and ILP32, and you did n= ot > >>> provided the compat shims. > >> > >> Really? How so? All of the fields have the same width on 64 and 32 > >> bit archs, and there shouldn't be any need for padding, either. > > Sorry, you are right. > > > >> > >>> > >>>> +/* Get the file's bmap info for the logical block bn */ > >>>> +#define FIOBMAP2 _IOWR('f', 99, struct fiobmap2_arg) > >>>> > >>>> #ifdef _KERNEL > >>>> #ifdef COMPAT_FREEBSD32 > >>>> > >>>> Modified: head/sys/ufs/ufs/ufs_bmap.c > >>>> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > >>>> --- head/sys/ufs/ufs/ufs_bmap.c Thu Jun 20 13:59:46 2019 > (r349230) > >>>> +++ head/sys/ufs/ufs/ufs_bmap.c Thu Jun 20 14:13:10 2019 > (r349231) > >>>> @@ -200,12 +200,15 @@ ufs_bmaparray(vp, bn, bnp, nbp, runp, runb) > >>>> *bnp =3D blkptrtodb(ump, ip->i_din2->di_extb[-1= - > bn]); > >>>> if (*bnp =3D=3D 0) > >>>> *bnp =3D -1; > >>>> - if (nbp =3D=3D NULL) > >>>> - panic("ufs_bmaparray: mapping ext > data"); > >>>> + if (nbp =3D=3D NULL) { > >>>> + /* indirect block not found */ > >>>> + return (EINVAL); > >>>> + } > >>> This (and next chunk) loose useful checks for in-kernel interfaces. > >> > >> mckusick disagrees with you on this. He thought that changing the > >> panics into errors was a good idea. Note that ufs_bmap was already > >> capable of returning errors that not all callers check. > > For user-callable code, this is of course the requirement. But for > in-kernel > > use, IMO it makes the interfaces loose. > > I=E2=80=99m firmly in agreement on changing panics into errors. When cal= lers are > encountered that don=E2=80=99t properly check and propagate the errors, t= hey > should be fixed too. I=E2=80=99m not saying that it=E2=80=99s an easy pr= ocess and that > we should just delete all panics, but it=E2=80=99s a goal that should be = vigorously > pursued. > > Scott > > > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpW_nD-B%2BKCb25Pik7SYWntFnsh9huVuHEzspXG2bQCVfA>