On Mon, May 17, 2010 at 9:20 PM, Kevin Wolf <kwolf@xxxxxxxxxx> wrote:
> Am 17.05.2010 14:19, schrieb MORITA Kazutaka:
>> At Mon, 17 May 2010 13:08:08 +0200,
>> Kevin Wolf wrote:
>>>
>>> Am 17.05.2010 12:19, schrieb MORITA Kazutaka:
>>>>
>>>> int bdrv_snapshot_goto(BlockDriverState *bs,
>>>> const char *snapshot_id)
>>>> {
>>>> BlockDriver *drv = bs->drv;
>>>> + int ret, open_ret;
>>>> +
>>>> if (!drv)
>>>> return -ENOMEDIUM;
>>>> - if (!drv->bdrv_snapshot_goto)
>>>> - return -ENOTSUP;
>>>> - return drv->bdrv_snapshot_goto(bs, snapshot_id);
>>>> + if (drv->bdrv_snapshot_goto)
>>>> + return drv->bdrv_snapshot_goto(bs, snapshot_id);
>>>> +
>>>> + if (bs->file) {
>>>> + drv->bdrv_close(bs);
>>>> + ret = bdrv_snapshot_goto(bs->file, snapshot_id);
>>>> + open_ret = drv->bdrv_open(bs, bs->open_flags);
>>>> + if (open_ret < 0) {
>>>> + bdrv_delete(bs);
>>>
>>> I think you mean bs->file here.
>>>
>>> Kevin
>>
>> This is an error of re-opening the format driver, so what we should
>> delete here is not bs->file but bs, isn't it? If we failed to open bs
>> here, the drive doesn't seem to work anymore.
>
> But bdrv_delete means basically free it. This is almost guaranteed to
> lead to crashes because that BlockDriverState is still in use in other
> places.
>
> One additional case of use after free is in the very next line:
>
>>>> + bs->drv = NULL;
>
> You can't do that when bs is freed, obviously. But I think just setting
> bs->drv to NULL without bdrv_deleting it before is the better way. It
> will fail any requests (with -ENOMEDIUM), but can't produce crashes.
> This is also what bdrv_commit does in such situations.
>
> In this state, we don't access the underlying file any more, so we could
> delete bs->file - this is why I thought you actually meant to do that.
>
I'm sorry for the confusion. I understand what we should do here.
I'll fix it for the next post.
Thanks,
Kazutaka
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
