Re: [PATCH v7 37/47] qemu-img: Use child access functions

[Max Reitz]

On 21.08.20 17:29, Kevin Wolf wrote:
> Am 25.06.2020 um 17:22 hat Max Reitz geschrieben:
>> This changes iotest 204's output, because blkdebug on top of a COW node
>> used to make qemu-img map disregard the rest of the backing chain (the
>> backing chain was broken by the filter).  With this patch, the
>> allocation in the base image is reported correctly.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
>> @@ -3437,6 +3441,7 @@ static int img_rebase(int argc, char **argv)
>>      uint8_t *buf_old = NULL;
>>      uint8_t *buf_new = NULL;
>>      BlockDriverState *bs = NULL, *prefix_chain_bs = NULL;
>> +    BlockDriverState *unfiltered_bs;
>>      char *filename;
>>      const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
>>      int c, flags, src_flags, ret;
>> @@ -3571,6 +3576,8 @@ static int img_rebase(int argc, char **argv)
>>      }
>>      bs = blk_bs(blk);
>>  
>> +    unfiltered_bs = bdrv_skip_filters(bs);
>> +
>>      if (out_basefmt != NULL) {
>>          if (bdrv_find_format(out_basefmt) == NULL) {
>>              error_report("Invalid format name: '%s'", out_basefmt);
>> @@ -3582,7 +3589,7 @@ static int img_rebase(int argc, char **argv)
>>      /* For safe rebasing we need to compare old and new backing file */
>>      if (!unsafe) {
>>          QDict *options = NULL;
>> -        BlockDriverState *base_bs = backing_bs(bs);
>> +        BlockDriverState *base_bs = bdrv_cow_bs(unfiltered_bs);
>>  
>>          if (base_bs) {
>>              blk_old_backing = blk_new(qemu_get_aio_context(),
>> @@ -3738,8 +3745,9 @@ static int img_rebase(int argc, char **argv)
>>                   * If cluster wasn't changed since prefix_chain, we don't 
>> need
>>                   * to take action
>>                   */
>> -                ret = bdrv_is_allocated_above(backing_bs(bs), 
>> prefix_chain_bs,
>> -                                              false, offset, n, &n);
>> +                ret = bdrv_is_allocated_above(bdrv_cow_bs(unfiltered_bs),
>> +                                              prefix_chain_bs, false,
>> +                                              offset, n, &n);
>>                  if (ret < 0) {
>>                      error_report("error while reading image metadata: %s",
>>                                   strerror(-ret));
> 
> img_rebase() has these additional calls:
> 
>     /* If the cluster is allocated, we don't need to take action */
>     ret = bdrv_is_allocated(bs, offset, n, &n);
> 
> And:
> 
>     if (out_baseimg && *out_baseimg) {
>         ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt);
>     } else {
>         ret = bdrv_change_backing_file(bs, NULL, NULL);
>     }
> 
> Shouldn't they use unfiltered_bs?

Oh, yes, the second one definitely.

As for the first one, I don’t think there’s a difference.  But why not,
we really want to query unfiltered_bs, so it’s better to do so
explicitly than through the implicit fall-through behavior of block_status.

Max

signature.asc
Description: OpenPGP digital signature