From owner-svn-src-head@freebsd.org Fri Jun 28 09:23:09 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1F38C15DD082; Fri, 28 Jun 2019 09:23:09 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: from mail-io1-f65.google.com (mail-io1-f65.google.com [209.85.166.65]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 919E46A003; Fri, 28 Jun 2019 09:23:08 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: by mail-io1-f65.google.com with SMTP id u19so2560315ior.9; Fri, 28 Jun 2019 02:23:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:reply-to :from:date:message-id:subject:to:cc; bh=I53FWSJHImXvmyYxBTkIAeHd9Qlt9XXJzqOYRxhctxs=; b=H9qbsFSUMo5XZ5/8f4nUr2s2Fpp4xchrM4t4xX6jJRagGfzW6oOG1P4vXHOaSmJNgJ KnCPfvN/cokcXwVtZk9Pl+JZ5a/P+0a0ZZR+ZWwtXxregI+hLM3VNQmDcKoj9PFQJTuz 4OTRKuTesSzIV0WP9VLg/OCsrfAGxHLbEJlX8vYNmxDb2WsS29V+LdLANGIW0NngG+72 ta3kflARM3GAQ+KlWgGnGsjcSUqagj8EkxLgPbi6UXHSt8w71Iyrkrg/+CNy6pU5rZZH eG1N6W11vQWeXRi6NUTAhDiryrU7eBnJ5qb17ZnpOeD/vv0znmC+Su2GVnFnUUiar8Nv DHDA== X-Gm-Message-State: APjAAAWz8EiUWCSdHu7G0zU7W3xcQGqZVj2f9Ahvl9TjEh2P+N8PaYs/ Ht/qGXUTm5kaSYn3dGf5CVUleA3mBj8= X-Google-Smtp-Source: APXvYqxTmJctPSr5Ls84RTS7z2oIxZhHH1SmyfzRndSPsp8Ytqc/aXreesXvqsq5zIE1+Nuh9Q3kvA== X-Received: by 2002:a5e:8518:: with SMTP id i24mr9502919ioj.149.1561713781236; Fri, 28 Jun 2019 02:23:01 -0700 (PDT) Received: from mail-io1-f47.google.com (mail-io1-f47.google.com. [209.85.166.47]) by smtp.gmail.com with ESMTPSA id v26sm1415272iom.88.2019.06.28.02.22.58 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Fri, 28 Jun 2019 02:22:59 -0700 (PDT) Received: by mail-io1-f47.google.com with SMTP id e3so11003796ioc.12; Fri, 28 Jun 2019 02:22:58 -0700 (PDT) X-Received: by 2002:a5d:8ccc:: with SMTP id k12mr9757897iot.141.1561713778598; Fri, 28 Jun 2019 02:22:58 -0700 (PDT) MIME-Version: 1.0 References: <201906201413.x5KEDB5u010923@repo.freebsd.org> <20190627191843.GQ8697@kib.kiev.ua> <20190627210940.GR8697@kib.kiev.ua> <0988866D-918B-4800-ADE7-938E1EBD729C@samsco.org> In-Reply-To: <0988866D-918B-4800-ADE7-938E1EBD729C@samsco.org> Reply-To: cem@freebsd.org From: Conrad Meyer Date: Fri, 28 Jun 2019 11:22:47 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r349231 - in head/sys: kern sys ufs/ufs To: Scott Long Cc: Alan Somers , Konstantin Belousov , src-committers , svn-src-all , svn-src-head X-Rspamd-Queue-Id: 919E46A003 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.99 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.99)[-0.988,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; TAGGED_FROM(0.00)[] Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 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: Fri, 28 Jun 2019 09:23:09 -0000 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 wrote: > > > > On Jun 27, 2019, at 3:09 PM, Konstantin Belousov > 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 > > > >