Re: [Qemu-devel] Re: [RFC PATCH v3 2/3] block: call the snapshot handlers of the protocol drivers
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.
Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [RFC PATCH v3 2/3] block: call the snapshot handlers of the protocol drivers
On Mon, May 17, 2010 at 9:20 PM, Kevin Wolf kw...@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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
