Re: [Qemu-block] [PATCH 2/2] block/io: refactor padding

[Kevin Wolf]

Am 31.05.2019 um 16:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 31.05.2019 13:51, Stefan Hajnoczi wrote:
> > On Tue, May 28, 2019 at 11:45:44AM +0300, Vladimir Sementsov-Ogievskiy 
> > wrote:
> >> We have similar padding code in bdrv_co_pwritev,
> >> bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify
> >> it.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> >> ---
> >>   block/io.c | 344 ++++++++++++++++++++++++++++-------------------------
> > 
> > Hmmm...this adds more lines than it removes.  O_o
> 
> It's near to be the same size, and keep in mind big comment.

If you take the whole series into account, it looks even less
favourable, despite some comments:

3 files changed, 273 insertions(+), 165 deletions(-)

> > 
> > Merging a change like this can still be useful but there's a risk of
> > making the code harder to understand due to the additional layers of
> > abstraction.
> 
> It's a preparation for adding qiov_offset parameter to read/write path. Seems
> correct to unify similar things, which I'm going to change. And I really want
> to make code more understandable than it was.. But my view is not general
> of course.

Depending on the changes you're going to make (e.g. adding more users of
the same functionality, or making the duplicated code much larger), this
can be a good justification even if the code size increases.

I'd suggest to add the explanation (like 'This is in preparation for
...') to the commit message then.

Kevin