linux-kernel - Re: [PATCH v3 02/13] fiemap: update fiemap_fill_next

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3bvnvw5lvlraveup3du7esp3v54wissngudpov3xzneajo2fji@hbqk52z2xp2z>
Date: Fri, 5 Apr 2024 15:06:43 -0400
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Andreas Dilger <adilger@...ger.ca>
Cc: Sweet Tea Dorminy <sweettea-kernel@...miny.me>, 
	Jonathan Corbet <corbet@....net>, Brian Foster <bfoster@...hat.com>, Chris Mason <clm@...com>, 
	Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>, 
	Jaegeuk Kim <jaegeuk@...nel.org>, Chao Yu <chao@...nel.org>, 
	Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, 
	Mickaël Salaün <mic@...ikod.net>, linux-doc@...r.kernel.org, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, linux-bcachefs@...r.kernel.org, 
	linux-btrfs <linux-btrfs@...r.kernel.org>, linux-f2fs-devel@...ts.sourceforge.net, 
	linux-fsdevel <linux-fsdevel@...r.kernel.org>, kernel-team@...a.com
Subject: Re: [PATCH v3 02/13] fiemap: update fiemap_fill_next_extent()
 signature

On Fri, Apr 05, 2024 at 01:05:01PM -0600, Andreas Dilger wrote:
> On Apr 3, 2024, at 1:22 AM, Sweet Tea Dorminy <sweettea-kernel@...miny.me> wrote:
> > 
> > Update the signature of fiemap_fill_next_extent() to allow passing a
> > physical length. Update all callers to pass a 0 physical length -- since
> > none set the EXTENT_HAS_PHYS_LEN flag, this value doesn't matter.
> 
> Patch-structure-wise, it doesn't make sense to me to change all of the callers
> to pass "0" as the argument to this function, and then submit a whole series
> of per-filesystem patches that sets only FIEMAP_EXTENT_HAS_PHYS_LEN (but also
> passes phys_len = 0, which is wrong AFAICS).
> 
> A cleaner approach would be to rename the existing fiemap_fill_next_extent()
> to fiemap_fill_next_extent_phys() that takes phys_len as an argument, and then
> add a simple wrapper until all of the filesystems are updated:
> 
> #define fiemap_fill_next_extent(info, logical, phys, log_len, flags, dev) \
>    fiemap_fill_next_extent_phys(info, logical, phys, log_len, 0, flags, dev)
> 
> Then the per-filesystem patches would involve changing over the callers to
> use fiemap_fill_next_extent_phys() and setting FIEMAP_EXTENT_HAS_PHYS_LEN.

Cleaner still would be to just have the callers initiaize and pass a
struct fiemap_extent instead of passing around a whole bunch of integer
parameters.

You get more explicit naming, better typesafety - functions with a bunch
of integer parametrs are not great from a type safety POV - and you can
add and pass fields to fiemap_extent without having to update callers
that aren't using it.