Re: [PATCH v2 7/7] block/block-copy: hide structure definitions

[Vladimir Sementsov-Ogievskiy]

17.02.2020 17:04, Max Reitz wrote:
> On 27.11.19 19:08, Vladimir Sementsov-Ogievskiy wrote:
> > Hide structure definitions and add explicit API instead, to keep an
> > eye on the scope of the shared fields.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   include/block/block-copy.h | 57 +++------------------------------
> >   block/backup-top.c         |  6 ++--
> >   block/backup.c             | 27 ++++++++--------
> >   block/block-copy.c         | 64 ++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 86 insertions(+), 68 deletions(-)
>
> [...]
>
> > diff --git a/block/backup.c b/block/backup.c
> > index cf62b1a38c..acab0d08da 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
>
> [...]
>
> > @@ -458,6 +458,7 @@ BlockJob *backup_job_create(const char *job_id, 
> > BlockDriverState *bs,
> >       job->sync_bitmap = sync_bitmap;
> >       job->bitmap_mode = bitmap_mode;
> >       job->bcs = bcs;
> > +    job->bcs_bitmap = block_copy_dirty_bitmap(bcs);
>
> It seems a bit weird to me to store a pointer to the BCS-owned bitmap
> here, because, well, it’s a BCS-owned object, and just calling
> block_copy_dirty_bitmap() every time wouldn’t be prohibitively expensive.
>
> I feel sufficiently bad about this to warrant not giving an R-b, but I
> know I shouldn’t withhold an R-b over this, so:
>
> Reviewed-by: Max Reitz <address@hidden>

Hmm, actually, I tend to agree with you. Why did I write it so? I'll look and 
may be change it
to block_copy_dirty_bitmap() calls every time.

Thanks for reviewing!

> >       job->cluster_size = cluster_size;
> >       job->len = len;


--
Best regards,
Vladimir