[Sheepdog] [Qemu-devel] Re: [RFC PATCH v3 2/3] block: call the snapshot handlers of the protocol drivers

[MORITA Kazutaka]

On Mon, May 17, 2010 at 9:20 PM, Kevin Wolf <kwolf at redhat.com> 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