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

[Vladimir Sementsov-Ogievskiy]

31.05.2019 18:49, Kevin Wolf wrote:
> 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
> 

I think, I'll resend the whole series with this preparation next week.

-- 
Best regards,
Vladimir