Re: [Qemu-devel] [Qemu-block] [PULL 76/76] block: move I/O request processing to block/io.c

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2545 bytes --]

Am 28.04.2015 um 19:15 hat Eric Blake geschrieben:
> On 04/28/2015 09:00 AM, Kevin Wolf wrote:
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > The block.c file has grown to over 6000 lines.  It is time to split this
> > file so there are fewer conflicts and the code is easier to maintain.
> > 
> > Extract I/O request processing code:
> >  * Read
> >  * Write
> >  * Zero writes and making the image empty
> >  * Flush
> >  * Discard
> >  * ioctl
> >  * Tracked requests and queuing
> >  * Throttling and copy-on-read
> >  * Block status and allocated functions
> >  * Refreshing block limits
> >  * Reading/writing vmstate
> >  * qemu_blockalign() and friends
> > 
> > The patch simply moves code from block.c into block/io.c.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c             | 3394 +++++++--------------------------------------------
> >  block/Makefile.objs |    2 +-
> >  block/io.c          | 2540 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 2982 insertions(+), 2954 deletions(-)
> >  create mode 100644 block/io.c
> 
> Kevin, compare your diff to Stefan's:
> 
> > ---
> >  block.c             | 2512 --------------------------------------------------
> >  block/Makefile.objs |    2 +-
> >  block/io.c          | 2540 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 2541 insertions(+), 2513 deletions(-)
> >  create mode 100644 block/io.c
> 
> The difference is that Stefan has done 'git config diff.algorithm
> patience', which makes for MUCH more legible diffs on a code motion
> patch.  http://wiki.qemu.org/Contribute/SubmitAPatch mentions this as a
> hint for a nicer setup.

Indeed, it does. I used --patience for reviewing the patch but neglected
to add it when sending the pull request. That said, I don't think I
could have enabled it for a single patch, and I'm not sure if it's
universally better (if so, why wouldn't it be the default?)

> At any rate, you've already sent the pull request, so I'm probably too
> late; but for the record, here's why I would add:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> when reviewing Stefan's version of the patch (the two versions give the
> same end result, but my review technique falls flat on Kevin's replay of
> the patch)

You could apply both series and diff the result if you wanted.

But this one was easy enough to review on my own (...famous last words?)

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]