Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/3] block: add default implementations for bdrv_co_get_block_status()

[Eric Blake]

On 06/29/2017 01:43 PM, Manos Pitsidianakis wrote:
> bdrv_co_get_block_status_from_file() and
> bdrv_co_get_block_status_from_backing() set *file to bs->file and
> bs->backing respectively, so that bdrv_co_get_block_status() can recurse
> to them. Future block drivers won't have to duplicate code to implement
> this.
> 
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Manos Pitsidianakis <address@hidden>
> ---
>  block/blkdebug.c          | 12 +-----------
>  block/commit.c            | 12 +-----------
>  block/io.c                | 24 ++++++++++++++++++++++++
>  block/mirror.c            | 12 +-----------
>  include/block/block_int.h | 16 ++++++++++++++++
>  5 files changed, 43 insertions(+), 33 deletions(-)

My [still-needs-rebasing] part 3 series converting
bdrv_co_get_block_status to byte-based will be impacted by this, but I'd
rather yours go in first and I can deal with the rebase fallout.


> +++ b/include/block/block_int.h
> @@ -945,6 +945,22 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
> BdrvChild *c,
>                                 uint64_t perm, uint64_t shared,
>                                 uint64_t *nperm, uint64_t *nshared);
>  
> +/*
> + * Default implementation for drivers to pass bdrv_co_get_block_status() to
> + * their file

I would have ended with '.'

> + * */

Looks odd.  Just use ' */', not ' * */'

> +int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
> +                                                     int64_t sector_num,
> +                                                     int nb_sectors, int 
> *pnum,
> +                                                     BlockDriverState 
> **file);

Indentation is unusual (not necessarily bad, since you are up against
80-column limits).

> +/*
> + * Default implementation for drivers to pass bdrv_co_get_block_status() to
> + * their backing file
> + * */
> +int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState 
> *bs,
> +                                                     int64_t sector_num,
> +                                                     int nb_sectors, int 
> *pnum,
> +                                                     BlockDriverState 
> **file);

Ditto.

Making changes according to what I mentioned is minor, so you can add:
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature