Re: [PATCH 05/21] block: rename backup-top to copy-before-write

[Max Reitz <mreitz@redhat.com>]

On 17.05.21 21:42, Vladimir Sementsov-Ogievskiy wrote:
> 17.05.2021 19:05, Max Reitz wrote:
>> On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
>>> We are going to convert backup_top to full featured public filter,
>>> which can be used in separate of backup job. Start from renaming from
>>> "how it used" to "what it does".
>>
>> Is this safe?  The name was externally visible in queries after all. 
>> (I’m not saying it is unsafe, I just don’t know and would like to know 
>> whether you’ve considered this already.)
>>
>> (Regardless, renaming files and so on is fine, of course.)
> 
> Hmmm. I don't know.
> 
> It was visible yes.. But we've never documented it. And if someone 
> depends on name of the format of the filter automatically inserted 
> during backup job, it's a kind of "undocumented feature" use..
> 
> Another change I is changing child from backing to file in 11, from this 
> point of view it's unsafe too. But ше even more reasonable than good 
> name: having all public filters behave similar is a very good thing.
> 
> So, may be it a bit risky, but I think good interface worth that risk. 
> And we always can say "sorry guys, but that was not documented, we 
> didn't promise anything".

OK.

Max

> But I'm OK to go on with "backup-top" and "backing", is someone has 
> strict opinion about this.
> 
>>
>>> While updating comments in 283 iotest, drop and rephrase also things
>>> about ".active", as this field is now dropped, and filter doesn't have
>>> "inactive" mode.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/{backup-top.h => copy-before-write.h} |  28 +++---
>>>   block/backup.c                              |  22 ++---
>>>   block/{backup-top.c => copy-before-write.c} | 100 ++++++++++----------
>>>   MAINTAINERS                                 |   4 +-
>>>   block/meson.build                           |   2 +-
>>>   tests/qemu-iotests/283                      |  35 +++----
>>>   tests/qemu-iotests/283.out                  |   4 +-
>>>   7 files changed, 95 insertions(+), 100 deletions(-)
>>>   rename block/{backup-top.h => copy-before-write.h} (56%)
>>>   rename block/{backup-top.c => copy-before-write.c} (62%)
>>
>> [...]
>>
>>> diff --git a/block/backup-top.c b/block/copy-before-write.c
>>> similarity index 62%
>>> rename from block/backup-top.c
>>> rename to block/copy-before-write.c
>>> index 425e3778be..40e91832d7 100644
>>> --- a/block/backup-top.c
>>> +++ b/block/copy-before-write.c
>>
>> [...]
>>
>>> @@ -32,25 +32,25 @@
>>
>> [...]
>>
>>> -static coroutine_fn int backup_top_cbw(BlockDriverState *bs, 
>>> uint64_t offset,
>>> -                                       uint64_t bytes, 
>>> BdrvRequestFlags flags)
>>> +static coroutine_fn int cbw_cbw(BlockDriverState *bs, uint64_t offset,
>>> +                                uint64_t bytes, BdrvRequestFlags flags)
>>
>> I’m sure you noticed it, too, but cbw_cbw() is weird.  Perhaps 
>> cbw_do_cbw() at least?
>>
> 
> OK. Maybe even cbw_do_copy_before_write()
> 
>