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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1415 bytes --]

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 <vsementsov@virtuozzo.com>
> ---
>  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 <mreitz@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]