linux-kernel - Re: [PATCH v4 05/12] vfio_pci: duplicate vfio

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: <20200115081224.215a994c@w520.home>
Date:   Wed, 15 Jan 2020 08:12:24 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Cornelia Huck <cohuck@...hat.com>
Cc:     Liu Yi L <yi.l.liu@...el.com>, kwankhede@...dia.com,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        kevin.tian@...el.com, joro@...tes.org, peterx@...hat.com,
        baolu.lu@...ux.intel.com
Subject: Re: [PATCH v4 05/12] vfio_pci: duplicate vfio_pci.c

On Wed, 15 Jan 2020 12:03:00 +0100
Cornelia Huck <cohuck@...hat.com> wrote:

> On Tue,  7 Jan 2020 20:01:42 +0800
> Liu Yi L <yi.l.liu@...el.com> wrote:
> 
> > This patch has no code change, just a file copy. In following patches,
> > vfio_pci_common.c will be modified to only include the common functions
> > and related static functions in original vfio_pci.c. Meanwhile, vfio_pci.c
> > will be modified to only include vfio-pci module specific codes.
> > 
> > Cc: Kevin Tian <kevin.tian@...el.com>
> > Cc: Lu Baolu <baolu.lu@...ux.intel.com>
> > Signed-off-by: Liu Yi L <yi.l.liu@...el.com>
> > ---
> >  drivers/vfio/pci/vfio_pci_common.c | 1708 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 1708 insertions(+)
> >  create mode 100644 drivers/vfio/pci/vfio_pci_common.c  
> 
> This whole procedure of "let's copy the file and rip out unneeded stuff
> later" looks very ugly to me, especially if I'd come across it in the
> future, e.g. during a bisect. This patch only adds a file that is not
> compiled, and later changes will be "rip out unwanted stuff from
> vfio_pci_common.c" instead of the more positive "move common stuff to
> vfio_pci_common.c". I think refactoring/moving interfaces/code that it
> makes sense to share makes this more reviewable, both now and in the
> future.

I think this comes largely at my request from previous reviews.  It's
very easy to apply this patch and diff the files to see that nothing
has changed, then review the subsequent patch to see that code is only
added or removed to check that there are no actual code changes.  If we
just selectively move code then I think it's left to our inspection to
verify nothing has changed.  Maybe this is a dummy step in a bisect,
but I don't see that you lose any granularity.  Thanks,

Alex