Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-5-27 23:25, Kevin Wolf 写道:
> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
>> From: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> The bs_snapshots global variable points to the BlockDriverState which
>> will be used to save vmstate.  This is really a savevm.c concept but was
>> moved into block.c:bdrv_snapshots() when it became clear that hotplug
>> could result in a dangling pointer.
>>
>> While auditing the block layer's global state I came upon bs_snapshots
>> and realized that a variable is not necessary here.  Simply find the
>> first BlockDriverState capable of internal snapshots each time this is
>> needed.
>>
>> The behavior of bdrv_snapshots() is preserved across hotplug because new
>> drives are always appended to the bdrv_states list.  This means that
>> calling the new find_vmstate_bs() function is idempotent - it returns
>> the same BlockDriverState unless it was hot-unplugged.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block.c               |   28 ----------------------------
>>   include/block/block.h |    1 -
>>   savevm.c              |   19 +++++++++++++++----
>>   3 files changed, 15 insertions(+), 33 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 3f87489..478a3b2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -99,9 +99,6 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
>>   static QLIST_HEAD(, BlockDriver) bdrv_drivers =
>>       QLIST_HEAD_INITIALIZER(bdrv_drivers);
>>
>> -/* The device to use for VM snapshots */
>> -static BlockDriverState *bs_snapshots;
>> -
>>   /* If non-zero, use only whitelisted block drivers */
>>   static int use_bdrv_whitelist;
>>
>> @@ -1357,9 +1354,6 @@ void bdrv_close(BlockDriverState *bs)
>>       notifier_list_notify(&bs->close_notifiers, bs);
>>
>>       if (bs->drv) {
>> -        if (bs == bs_snapshots) {
>> -            bs_snapshots = NULL;
>> -        }
>>           if (bs->backing_hd) {
>>               bdrv_delete(bs->backing_hd);
>>               bs->backing_hd = NULL;
>> @@ -1591,7 +1585,6 @@ void bdrv_delete(BlockDriverState *bs)
>>
>>       bdrv_close(bs);
>>
>> -    assert(bs != bs_snapshots);
>>       g_free(bs);
>>   }
>>
>> @@ -1635,9 +1628,6 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
>>   {
>>       bs->dev_ops = ops;
>>       bs->dev_opaque = opaque;
>> -    if (bdrv_dev_has_removable_media(bs) && bs == bs_snapshots) {
>> -        bs_snapshots = NULL;
>> -    }
>
> This hunk isn't replaced by any other code. If I understand correctly
> what it's doing, it prevented you from saving the VM state to a
> removable device, which would be allowed after this patch.
>
> Is this a change we want to make? Why isn't it described in the commit
> message?
   How about adding it back in find_vmstate_bs()?

>
> Kevin
>


-- 
Best Regards

Wenchao Xia