linux-kernel - Re: [PATCH v5 02/12] common/rc: Add _require_fio

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <aMEc2gW2WA0kYUks@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
Date: Wed, 10 Sep 2025 12:08:18 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: Zorro Lang <zlang@...hat.com>
Cc: John Garry <john.g.garry@...cle.com>, fstests@...r.kernel.org,
        Ritesh Harjani <ritesh.list@...il.com>, djwong@...nel.org,
        tytso@....edu, linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-ext4@...r.kernel.org
Subject: Re: [PATCH v5 02/12] common/rc: Add _require_fio_version helper

On Wed, Sep 10, 2025 at 02:07:15PM +0800, Zorro Lang wrote:
> On Tue, Sep 09, 2025 at 02:32:36PM +0530, Ojaswin Mujoo wrote:
> > On Tue, Sep 09, 2025 at 08:26:52AM +0100, John Garry wrote:
> > > On 09/09/2025 08:16, Ojaswin Mujoo wrote:
> > > > > > This requires the user to know the version which corresponds to the feature.
> > > > > > Is that how things are done for other such utilities and their versions vs
> > > > > > features?
> > > > > > 
> > > > > > I was going to suggest exporting something like
> > > > > > _require_fio_atomic_writes(), and _require_fio_atomic_writes() calls
> > > > > > _require_fio_version() to check the version.
> > > > > (Sorry, I made a half reply in my last email)
> > > > > 
> > > > > This looks better than only using _require_fio_version. But the nature is still
> > > > > checking fio version. If we don't have a better idea to check if fio really
> > > > > support atomic writes, the _require_fio_version is still needed.
> > > > > Or we rename it to "__require_fio_version" (one more "_"), to mark it's
> > > > > not recommended using directly. But that looks a bit like a trick 😂
> > > > > 
> > > > > Thanks,
> > > > > Zorro
> > > > Hey Zorro, I agree with your points that version might not be the best
> > > > indicator esp for downstream software, but at this point I'm unsure
> > > > what's the workaround.
> 
> Hi Ojaswin, I don't have better workaround than require_fio_version for now. I mean:
> 1) name _require_fio_version as __require_fio_version, to mark it's an internal function
>    of another common function.
> 2) only call __require_fio_version in _require_fio_atomic_writes for now, don't use it
>    in any test cases directly.

Got it, I'll make this change, thanks

> 
> > > > 
> > > > One thing that comes to mind is to let fio do the atomic write and use
> > > > the tracepoints to confirm if RWF_ATOMIC was passed, but that adds a lot
> > > > of dependency on tracing framework being present (im unsure if something
> > > > like this is used somewhere in xfstests before). Further it's messy to
> > > > figure out that out of all the IO fio command will do, which one to
> > > > check for RWF_ATOMIC.
> > > > 
> > > > It can be done I suppose but is this sort of complexity something we
> > > > want to add is the question. Or do we just go ahead with the version
> > > > check.
> > > 
> > > I think that just checking the version is fine for this specific feature.
> > > But I still also think that versioning should be hidden from the end user,
> > > i.e. we should provide a helper like _require_fio_atomic_writes
> > 
> > Sure, I'm okay. @Zorro, does that sound okay to you?
> 
> Sure, that's what I tried to say as above, sorry if I made you misunderstand :)

Right I was just confirming. Thanks for the review :)

Regards,
ojaswin
> 
> Thanks,
> Zorro
> 
> > > 
> > > thanks,
> > > John
> > 
>