Re: [Qemu-block] [PATCH v5 22/38] block: Prepare for NULL BDS

[Max Reitz]

On 07.10.2015 12:43, Kevin Wolf wrote:
> Am 18.09.2015 um 17:22 hat Max Reitz geschrieben:
>> blk_bs() will not necessarily return a non-NULL value any more (unless
>> blk_is_available() is true or it can be assumed to otherwise, e.g.
>> because it is called immediately after a successful blk_new_with_bs() or
>> blk_new_open()).
>>
>> Signed-off-by: Max Reitz <address@hidden>
> 
>> @@ -1922,8 +1939,9 @@ void qmp_change_blockdev(const char *device, const 
>> char *filename,
>>          return;
>>      }
>>      bs = blk_bs(blk);
>> +    new_bs = !bs;
>>  
>> -    aio_context = bdrv_get_aio_context(bs);
>> +    aio_context = blk_get_aio_context(blk);
>>      aio_context_acquire(aio_context);
>>  
>>      eject_device(blk, 0, &err);
>> @@ -1932,10 +1950,18 @@ void qmp_change_blockdev(const char *device, const 
>> char *filename,
>>          goto out;
>>      }
>>  
>> -    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
>> -    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
>> +    bdrv_flags = blk_is_read_only(blk) ? 0 : BDRV_O_RDWR;
>> +    bdrv_flags |= blk_get_root_state(blk)->open_flags & ~BDRV_O_RDWR;
>>  
>> -    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, format, NULL, errp);
>> +    qmp_bdrv_open_encrypted(&bs, filename, bdrv_flags, format, NULL, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +    } else if (new_bs) {
>> +        blk_insert_bs(blk, bs);
>> +        /* Has been sent automatically by bdrv_open() if blk_bs(blk) was not
>> +         * NULL */
>> +        blk_dev_change_media_cb(blk, true);
>> +    }
> 
> Was this change supposed to be in this patch? It looks like it's doing a
> bit more than just fixing blk_bs() == NULL cases.

Yes, it's supposed to be here. Before this patch, qmp_change_blockdev()
took the BDS from the specified BB, closed it (eject_device()) and
"re-opened" it (bdrv_open() in qmp_bdrv_open_encrypted()), and we're done.

Now, the BB does not have to have a BDS anymore. In this case
(new_bs == true) qmp_bdrv_open_encrypted() will now create a new BDS
(which is why now it needs to take a BDS ** instead of a plain BDS *)
and we have to then attach it to the BB.

The comment above blk_dev_change_media_cb() then explains why we need
that, too.

Later ("Implement change with basic operations") I'll drop it altogether
anyway, and then, since the "eject" part of "change" will then detach
the BDS from the BB, we'll always have the "need to attach a new BDS to
the BB" case.

Max

signature.asc
Description: OpenPGP digital signature