Re: [Qemu-devel] [PATCH v4 01/23] block: Allow NULL file for bdrv_get_block_status()

[Eric Blake]

On 09/25/2017 05:43 PM, John Snow wrote:
> 
> 
> On 09/13/2017 12:03 PM, Eric Blake wrote:
>> Not all callers care about which BDS owns the mapping for a given
>> range of the file.  This patch merely simplifies the callers by
>> consolidating the logic in the common call point, while guaranteeing
>> a non-NULL file to all the driver callbacks, for no semantic change.
>> The only caller that does not care about pnum is bdrv_is_allocated,
>> as invoked by vvfat; we can likewise add assertions that the rest
>> of the stack does not have to worry about a NULL pnum.
>>
>> Furthermore, this will also set the stage for a future cleanup: when
>> a caller does not care about which BDS owns an offset, it would be
>> nice to allow the driver to optimize things to not have to return
>> BDRV_BLOCK_OFFSET_VALID in the first place.  In the case of fragmented
>> allocation (for example, it's fairly easy to create a qcow2 image
>> where consecutive guest addresses are not at consecutive host
>> addresses), the current contract requires bdrv_get_block_status()
>> to clamp *pnum to the limit where host addresses are no longer
>> consecutive, but allowing a NULL file means that *pnum could be
>> set to the full length of known-allocated data.
>>
> 
> Function reads slightly worse for the wear now with all of the return
> logic handled at various places within, but unifying it might be even
> stranger, perhaps..
> 
> Let's see if I hate this more:
> 
> out:
> bdrv_dec_in_flight(bs);
> bdrv_dec_in_flight(bs);
> if (ret >= 0 && sector_num + *pnum == total_sectors) {
> ret |= BDRV_BLOCK_EOF;
> }
> early_out:
> if (file) {
> *file = local_file;
> }
> return ret;
> 
> 
> and then earlier in the function, we can just:
> 
> if (total_sectors < 0) {
>   ret = total_sectors;
>   goto early_out;
> }

Seems reasonable enough, I'll work that in to v5, since there are other
reasons to respin the series anyway.

> 
> It's only shed paint, though:
> 
> Reviewed-by: John Snow 
> 
> I'm looking at the rest of the series now, so please stand by.
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v4 01/23] block: Allow NULL file for bdrv_get_block_status()

[John Snow]

On 09/13/2017 12:03 PM, Eric Blake wrote:
> Not all callers care about which BDS owns the mapping for a given
> range of the file.  This patch merely simplifies the callers by
> consolidating the logic in the common call point, while guaranteeing
> a non-NULL file to all the driver callbacks, for no semantic change.
> The only caller that does not care about pnum is bdrv_is_allocated,
> as invoked by vvfat; we can likewise add assertions that the rest
> of the stack does not have to worry about a NULL pnum.
> 
> Furthermore, this will also set the stage for a future cleanup: when
> a caller does not care about which BDS owns an offset, it would be
> nice to allow the driver to optimize things to not have to return
> BDRV_BLOCK_OFFSET_VALID in the first place.  In the case of fragmented
> allocation (for example, it's fairly easy to create a qcow2 image
> where consecutive guest addresses are not at consecutive host
> addresses), the current contract requires bdrv_get_block_status()
> to clamp *pnum to the limit where host addresses are no longer
> consecutive, but allowing a NULL file means that *pnum could be
> set to the full length of known-allocated data.
> 
> Signed-off-by: Eric Blake 
> 
> ---
> v4: only context changes
> v3: rebase to recent changes (qcow2_measure), dropped R-b
> v2: use local variable and final transfer, rather than assignment
> of parameter to local
> [previously in different series]:
> v2: new patch, 
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05645.html
> ---
>  include/block/block_int.h | 10 ++
>  block/io.c| 44 
>  block/mirror.c|  3 +--
>  block/qcow2.c |  8 ++--
>  qemu-img.c| 10 --
>  5 files changed, 41 insertions(+), 34 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 55c5d573d4..7f71c585a0 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -202,10 +202,12 @@ struct BlockDriver {
>  int64_t offset, int bytes);
> 
>  /*
> - * Building block for bdrv_block_status[_above]. The driver should
> - * answer only according to the current layer, and should not
> - * set BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW.  See block.h
> - * for the meaning of _DATA, _ZERO, and _OFFSET_VALID.
> + * Building block for bdrv_block_status[_above] and
> + * bdrv_is_allocated[_above].  The driver should answer only
> + * according to the current layer, and should not set
> + * BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW.  See block.h
> + * for the meaning of _DATA, _ZERO, and _OFFSET_VALID.  The block
> + * layer guarantees non-NULL pnum and file.
>   */
>  int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
>  int64_t sector_num, int nb_sectors, int *pnum,
> diff --git a/block/io.c b/block/io.c
> index 8a0cd8835a..f250029395 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -695,7 +695,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags 
> flags)
>  {
>  int64_t target_sectors, ret, nb_sectors, sector_num = 0;
>  BlockDriverState *bs = child->bs;
> -BlockDriverState *file;
>  int n;
> 
>  target_sectors = bdrv_nb_sectors(bs);
> @@ -708,7 +707,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags 
> flags)
>  if (nb_sectors <= 0) {
>  return 0;
>  }
> -ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , );
> +ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , NULL);
>  if (ret < 0) {
>  error_report("error getting block status at sector %" PRId64 ": 
> %s",
>   sector_num, strerror(-ret));
> @@ -1755,8 +1754,9 @@ int64_t coroutine_fn 
> bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
>   * beyond the end of the disk image it will be clamped; if 'pnum' is set to
>   * the end of the image, then the returned value will include BDRV_BLOCK_EOF.
>   *
> - * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 
> 'file'
> - * points to the BDS which the sector range is allocated in.
> + * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and
> + * 'file' is non-NULL, then '*file' points to the BDS which the sector range
> + * is allocated in.
>   */
>  static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>   int64_t sector_num,
> @@ -1766,15 +1766,22 @@ static int64_t coroutine_fn 
> bdrv_co_get_block_status(BlockDriverState *bs,
>  int64_t total_sectors;
>  int64_t n;
>  int64_t ret, ret2;
> +BlockDriverState *local_file = NULL;
> 
> -*file = NULL;
> +assert(pnum);

nice assert..

>  total_sectors = bdrv_nb_sectors(bs);
>  if (total_sectors < 0) {
> +if (file) {
> +*file = NULL;

[Qemu-devel] [PATCH v4 01/23] block: Allow NULL file for bdrv_get_block_status()

[Eric Blake]

Not all callers care about which BDS owns the mapping for a given
range of the file.  This patch merely simplifies the callers by
consolidating the logic in the common call point, while guaranteeing
a non-NULL file to all the driver callbacks, for no semantic change.
The only caller that does not care about pnum is bdrv_is_allocated,
as invoked by vvfat; we can likewise add assertions that the rest
of the stack does not have to worry about a NULL pnum.

Furthermore, this will also set the stage for a future cleanup: when
a caller does not care about which BDS owns an offset, it would be
nice to allow the driver to optimize things to not have to return
BDRV_BLOCK_OFFSET_VALID in the first place.  In the case of fragmented
allocation (for example, it's fairly easy to create a qcow2 image
where consecutive guest addresses are not at consecutive host
addresses), the current contract requires bdrv_get_block_status()
to clamp *pnum to the limit where host addresses are no longer
consecutive, but allowing a NULL file means that *pnum could be
set to the full length of known-allocated data.

Signed-off-by: Eric Blake 

---
v4: only context changes
v3: rebase to recent changes (qcow2_measure), dropped R-b
v2: use local variable and final transfer, rather than assignment
of parameter to local
[previously in different series]:
v2: new patch, 
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05645.html
---
 include/block/block_int.h | 10 ++
 block/io.c| 44 
 block/mirror.c|  3 +--
 block/qcow2.c |  8 ++--
 qemu-img.c| 10 --
 5 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 55c5d573d4..7f71c585a0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -202,10 +202,12 @@ struct BlockDriver {
 int64_t offset, int bytes);

 /*
- * Building block for bdrv_block_status[_above]. The driver should
- * answer only according to the current layer, and should not
- * set BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW.  See block.h
- * for the meaning of _DATA, _ZERO, and _OFFSET_VALID.
+ * Building block for bdrv_block_status[_above] and
+ * bdrv_is_allocated[_above].  The driver should answer only
+ * according to the current layer, and should not set
+ * BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW.  See block.h
+ * for the meaning of _DATA, _ZERO, and _OFFSET_VALID.  The block
+ * layer guarantees non-NULL pnum and file.
  */
 int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
 int64_t sector_num, int nb_sectors, int *pnum,
diff --git a/block/io.c b/block/io.c
index 8a0cd8835a..f250029395 100644
--- a/block/io.c
+++ b/block/io.c
@@ -695,7 +695,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
 {
 int64_t target_sectors, ret, nb_sectors, sector_num = 0;
 BlockDriverState *bs = child->bs;
-BlockDriverState *file;
 int n;

 target_sectors = bdrv_nb_sectors(bs);
@@ -708,7 +707,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
 if (nb_sectors <= 0) {
 return 0;
 }
-ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , );
+ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , NULL);
 if (ret < 0) {
 error_report("error getting block status at sector %" PRId64 ": 
%s",
  sector_num, strerror(-ret));
@@ -1755,8 +1754,9 @@ int64_t coroutine_fn 
bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
  * beyond the end of the disk image it will be clamped; if 'pnum' is set to
  * the end of the image, then the returned value will include BDRV_BLOCK_EOF.
  *
- * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file'
- * points to the BDS which the sector range is allocated in.
+ * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and
+ * 'file' is non-NULL, then '*file' points to the BDS which the sector range
+ * is allocated in.
  */
 static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
  int64_t sector_num,
@@ -1766,15 +1766,22 @@ static int64_t coroutine_fn 
bdrv_co_get_block_status(BlockDriverState *bs,
 int64_t total_sectors;
 int64_t n;
 int64_t ret, ret2;
+BlockDriverState *local_file = NULL;

-*file = NULL;
+assert(pnum);
 total_sectors = bdrv_nb_sectors(bs);
 if (total_sectors < 0) {
+if (file) {
+*file = NULL;
+}
 return total_sectors;
 }

 if (sector_num >= total_sectors) {
 *pnum = 0;
+if (file) {
+*file = NULL;
+}
 return BDRV_BLOCK_EOF;
 }

@@ -1791,23 +1798,27 @@ static int64_t coroutine_fn