linux-kernel - Re: [PATCH v2 2/8] cdrom: factor out common open_for

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.21.1910261652580.8@nippy.intranet>
Date:   Sat, 26 Oct 2019 17:46:08 +1100 (AEDT)
From:   Finn Thain <fthain@...egraphics.com.au>
To:     Michal Suchánek <msuchanek@...e.de>
cc:     Christoph Hellwig <hch@...radead.org>, linux-scsi@...r.kernel.org,
        Jonathan Corbet <corbet@....net>, Jens Axboe <axboe@...nel.dk>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Eric Biggers <ebiggers@...gle.com>,
        "J. Bruce Fields" <bfields@...hat.com>,
        Benjamin Coddington <bcodding@...hat.com>,
        Hannes Reinecke <hare@...e.com>,
        Omar Sandoval <osandov@...com>, Ming Lei <ming.lei@...hat.com>,
        Damien Le Moal <damien.lemoal@....com>,
        Bart Van Assche <bvanassche@....org>,
        Tejun Heo <tj@...nel.org>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2 2/8] cdrom: factor out common open_for_* code

On Fri, 25 Oct 2019, Michal Such?nek wrote:

> On Thu, Oct 24, 2019 at 07:39:08PM -0700, Christoph Hellwig wrote:
> > On Thu, Oct 24, 2019 at 10:50:14AM +0200, Michal Such?nek wrote:
> > > Then I will get complaints I do unrelated changes and it's hard to
> > > review. The code gets removed later anyway.
> > 
> > If you refactor you you pretty much have a card blanche for the
> > refactored code and the direct surroundings.
> 
> This is different from what other reviewers say:
> 
> https://lore.kernel.org/lkml/1517245320.2687.14.camel@wdc.com/
> 

I don't see any inconsistency there. Both reviews are valuable.

In general, different reviewers may give contradictory advice. Reviewers 
probably even contradict themselves eventually. Yet it rarely happens that 
the same patch gets contradictory reviews. If it did, you might well 
complain.

> Either way, this code is removed in a later patch so this discussion is
> moot.
> 
> It makes sense to have a bisection point here in case something
> goes wrong but it is pointless to argue about the code structure
> inherited from the previous revision.

A patch may refactor some code only to have the next patch remove that 
code. This doesn't generally mean that the former patch is redundant.

The latter patch may end up committed and subsequently reverted. The 
latter patch may become easier to review because of the former. The former 
patch may be eligible for -stable. The former patch may be the result of 
an automatic process. And so on.

I don't know what Christoph had in mind here but he's usually right, so 
it's worth asking.

-- 

> 
> Thanks
> 
> Michal
>