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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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".

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()


-- 
Best regards,
Vladimir