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

[Vladimir Sementsov-Ogievskiy]

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