* [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto()
2017-11-20 14:50 [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps Kevin Wolf
@ 2017-11-20 14:50 ` Kevin Wolf
2017-11-20 16:07 ` Vladimir Sementsov-Ogievskiy
2017-11-20 14:50 ` [Qemu-devel] [PATCH for-2.11 2/3] block: Add errp to bdrv_all_goto_snapshot() Kevin Wolf
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2017-11-20 14:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, jsnow, vsementsov, den, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/snapshot.h | 3 ++-
block/snapshot.c | 21 ++++++++++++++++-----
qemu-img.c | 6 +++---
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index e5c0553115..aeb80405e8 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -57,7 +57,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id);
+ const char *snapshot_id,
+ Error **errp);
int bdrv_snapshot_delete(BlockDriverState *bs,
const char *snapshot_id,
const char *name,
diff --git a/block/snapshot.c b/block/snapshot.c
index be0743abac..9c941e638d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -177,18 +177,21 @@ int bdrv_snapshot_create(BlockDriverState *bs,
}
int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id)
+ const char *snapshot_id,
+ Error **errp)
{
BlockDriver *drv = bs->drv;
int ret, open_ret;
int64_t len;
if (!drv) {
+ error_setg(errp, "Block driver is closed");
return -ENOMEDIUM;
}
len = bdrv_getlength(bs);
if (len < 0) {
+ error_setg_errno(errp, -len, "Cannot get block device size");
return len;
}
/* We should set all bits in all enabled dirty bitmaps, because dirty
@@ -200,13 +203,18 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
bdrv_set_dirty(bs, 0, len);
if (drv->bdrv_snapshot_goto) {
- return drv->bdrv_snapshot_goto(bs, snapshot_id);
+ ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to load snapshot");
+ }
+ return ret;
}
if (bs->file) {
BlockDriverState *file;
QDict *options = qdict_clone_shallow(bs->options);
QDict *file_options;
+ Error *local_err = NULL;
file = bs->file->bs;
/* Prevent it from getting deleted when detached from bs */
@@ -220,12 +228,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
bdrv_unref_child(bs, bs->file);
bs->file = NULL;
- ret = bdrv_snapshot_goto(file, snapshot_id);
- open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
+ ret = bdrv_snapshot_goto(file, snapshot_id, errp);
+ open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
QDECREF(options);
if (open_ret < 0) {
bdrv_unref(file);
bs->drv = NULL;
+ /* A bdrv_snapshot_goto() error takes precedence */
+ error_propagate(errp, local_err);
return open_ret;
}
@@ -234,6 +244,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return ret;
}
+ error_setg(errp, "Block driver does not support snapshots");
return -ENOTSUP;
}
@@ -467,7 +478,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
aio_context_acquire(ctx);
if (bdrv_can_snapshot(bs)) {
- err = bdrv_snapshot_goto(bs, name);
+ err = bdrv_snapshot_goto(bs, name, NULL);
}
aio_context_release(ctx);
if (err < 0) {
diff --git a/qemu-img.c b/qemu-img.c
index 02a6e27beb..68b375f998 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2989,10 +2989,10 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_APPLY:
- ret = bdrv_snapshot_goto(bs, snapshot_name);
+ ret = bdrv_snapshot_goto(bs, snapshot_name, &err);
if (ret) {
- error_report("Could not apply snapshot '%s': %d (%s)",
- snapshot_name, ret, strerror(-ret));
+ error_reportf_err(err, "Could not apply snapshot '%s': ",
+ snapshot_name);
}
break;
--
2.13.6
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto()
2017-11-20 14:50 ` [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto() Kevin Wolf
@ 2017-11-20 16:07 ` Vladimir Sementsov-Ogievskiy
2017-11-20 16:23 ` Kevin Wolf
0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-20 16:07 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, eblake, jsnow, den, qemu-devel
20.11.2017 17:50, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/block/snapshot.h | 3 ++-
> block/snapshot.c | 21 ++++++++++++++++-----
> qemu-img.c | 6 +++---
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index e5c0553115..aeb80405e8 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -57,7 +57,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
> int bdrv_snapshot_create(BlockDriverState *bs,
> QEMUSnapshotInfo *sn_info);
> int bdrv_snapshot_goto(BlockDriverState *bs,
> - const char *snapshot_id);
> + const char *snapshot_id,
> + Error **errp);
> int bdrv_snapshot_delete(BlockDriverState *bs,
> const char *snapshot_id,
> const char *name,
> diff --git a/block/snapshot.c b/block/snapshot.c
> index be0743abac..9c941e638d 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -177,18 +177,21 @@ int bdrv_snapshot_create(BlockDriverState *bs,
> }
>
> int bdrv_snapshot_goto(BlockDriverState *bs,
> - const char *snapshot_id)
> + const char *snapshot_id,
> + Error **errp)
> {
> BlockDriver *drv = bs->drv;
> int ret, open_ret;
> int64_t len;
>
> if (!drv) {
> + error_setg(errp, "Block driver is closed");
> return -ENOMEDIUM;
> }
>
> len = bdrv_getlength(bs);
> if (len < 0) {
> + error_setg_errno(errp, -len, "Cannot get block device size");
> return len;
> }
> /* We should set all bits in all enabled dirty bitmaps, because dirty
> @@ -200,13 +203,18 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> bdrv_set_dirty(bs, 0, len);
>
> if (drv->bdrv_snapshot_goto) {
> - return drv->bdrv_snapshot_goto(bs, snapshot_id);
> + ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Failed to load snapshot");
> + }
> + return ret;
> }
>
> if (bs->file) {
> BlockDriverState *file;
> QDict *options = qdict_clone_shallow(bs->options);
> QDict *file_options;
> + Error *local_err = NULL;
>
> file = bs->file->bs;
> /* Prevent it from getting deleted when detached from bs */
> @@ -220,12 +228,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> bdrv_unref_child(bs, bs->file);
> bs->file = NULL;
>
> - ret = bdrv_snapshot_goto(file, snapshot_id);
> - open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
> + ret = bdrv_snapshot_goto(file, snapshot_id, errp);
> + open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
> QDECREF(options);
> if (open_ret < 0) {
> bdrv_unref(file);
> bs->drv = NULL;
> + /* A bdrv_snapshot_goto() error takes precedence */
you return open_ret from bdrv_open and err msg from bdrv_snapshot_goto.
the may not correspond to each other.
> + error_propagate(errp, local_err);
> return open_ret;
> }
>
> @@ -234,6 +244,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> return ret;
> }
>
> + error_setg(errp, "Block driver does not support snapshots");
> return -ENOTSUP;
> }
>
> @@ -467,7 +478,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>
> aio_context_acquire(ctx);
> if (bdrv_can_snapshot(bs)) {
> - err = bdrv_snapshot_goto(bs, name);
> + err = bdrv_snapshot_goto(bs, name, NULL);
> }
> aio_context_release(ctx);
> if (err < 0) {
> diff --git a/qemu-img.c b/qemu-img.c
> index 02a6e27beb..68b375f998 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2989,10 +2989,10 @@ static int img_snapshot(int argc, char **argv)
> break;
>
> case SNAPSHOT_APPLY:
> - ret = bdrv_snapshot_goto(bs, snapshot_name);
> + ret = bdrv_snapshot_goto(bs, snapshot_name, &err);
> if (ret) {
> - error_report("Could not apply snapshot '%s': %d (%s)",
> - snapshot_name, ret, strerror(-ret));
> + error_reportf_err(err, "Could not apply snapshot '%s': ",
> + snapshot_name);
> }
> break;
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto()
2017-11-20 16:07 ` Vladimir Sementsov-Ogievskiy
@ 2017-11-20 16:23 ` Kevin Wolf
2017-11-20 16:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2017-11-20 16:23 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, mreitz, eblake, jsnow, den, qemu-devel
Am 20.11.2017 um 17:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 20.11.2017 17:50, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > include/block/snapshot.h | 3 ++-
> > block/snapshot.c | 21 ++++++++++++++++-----
> > qemu-img.c | 6 +++---
> > 3 files changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> > index e5c0553115..aeb80405e8 100644
> > --- a/include/block/snapshot.h
> > +++ b/include/block/snapshot.h
> > @@ -57,7 +57,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
> > int bdrv_snapshot_create(BlockDriverState *bs,
> > QEMUSnapshotInfo *sn_info);
> > int bdrv_snapshot_goto(BlockDriverState *bs,
> > - const char *snapshot_id);
> > + const char *snapshot_id,
> > + Error **errp);
> > int bdrv_snapshot_delete(BlockDriverState *bs,
> > const char *snapshot_id,
> > const char *name,
> > diff --git a/block/snapshot.c b/block/snapshot.c
> > index be0743abac..9c941e638d 100644
> > --- a/block/snapshot.c
> > +++ b/block/snapshot.c
> > @@ -177,18 +177,21 @@ int bdrv_snapshot_create(BlockDriverState *bs,
> > }
> > int bdrv_snapshot_goto(BlockDriverState *bs,
> > - const char *snapshot_id)
> > + const char *snapshot_id,
> > + Error **errp)
> > {
> > BlockDriver *drv = bs->drv;
> > int ret, open_ret;
> > int64_t len;
> > if (!drv) {
> > + error_setg(errp, "Block driver is closed");
> > return -ENOMEDIUM;
> > }
> > len = bdrv_getlength(bs);
> > if (len < 0) {
> > + error_setg_errno(errp, -len, "Cannot get block device size");
> > return len;
> > }
> > /* We should set all bits in all enabled dirty bitmaps, because dirty
> > @@ -200,13 +203,18 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > bdrv_set_dirty(bs, 0, len);
> > if (drv->bdrv_snapshot_goto) {
> > - return drv->bdrv_snapshot_goto(bs, snapshot_id);
> > + ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
> > + if (ret < 0) {
> > + error_setg_errno(errp, -ret, "Failed to load snapshot");
> > + }
> > + return ret;
> > }
> > if (bs->file) {
> > BlockDriverState *file;
> > QDict *options = qdict_clone_shallow(bs->options);
> > QDict *file_options;
> > + Error *local_err = NULL;
> > file = bs->file->bs;
> > /* Prevent it from getting deleted when detached from bs */
> > @@ -220,12 +228,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > bdrv_unref_child(bs, bs->file);
> > bs->file = NULL;
> > - ret = bdrv_snapshot_goto(file, snapshot_id);
> > - open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
> > + ret = bdrv_snapshot_goto(file, snapshot_id, errp);
> > + open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
> > QDECREF(options);
> > if (open_ret < 0) {
> > bdrv_unref(file);
> > bs->drv = NULL;
> > + /* A bdrv_snapshot_goto() error takes precedence */
>
> you return open_ret from bdrv_open and err msg from bdrv_snapshot_goto.
> the may not correspond to each other.
Hm, yes. Is this a problem, though?
If it is, I guess we can change the return value below:
> > + error_propagate(errp, local_err);
> > return open_ret;
return ret < 0 ? ret : open_ret;
Would people prefer this?
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto()
2017-11-20 16:23 ` Kevin Wolf
@ 2017-11-20 16:31 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-20 16:31 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, mreitz, eblake, jsnow, den, qemu-devel
20.11.2017 19:23, Kevin Wolf wrote:
> Am 20.11.2017 um 17:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 20.11.2017 17:50, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>> include/block/snapshot.h | 3 ++-
>>> block/snapshot.c | 21 ++++++++++++++++-----
>>> qemu-img.c | 6 +++---
>>> 3 files changed, 21 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
>>> index e5c0553115..aeb80405e8 100644
>>> --- a/include/block/snapshot.h
>>> +++ b/include/block/snapshot.h
>>> @@ -57,7 +57,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
>>> int bdrv_snapshot_create(BlockDriverState *bs,
>>> QEMUSnapshotInfo *sn_info);
>>> int bdrv_snapshot_goto(BlockDriverState *bs,
>>> - const char *snapshot_id);
>>> + const char *snapshot_id,
>>> + Error **errp);
>>> int bdrv_snapshot_delete(BlockDriverState *bs,
>>> const char *snapshot_id,
>>> const char *name,
>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>> index be0743abac..9c941e638d 100644
>>> --- a/block/snapshot.c
>>> +++ b/block/snapshot.c
>>> @@ -177,18 +177,21 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>>> }
>>> int bdrv_snapshot_goto(BlockDriverState *bs,
>>> - const char *snapshot_id)
>>> + const char *snapshot_id,
>>> + Error **errp)
>>> {
>>> BlockDriver *drv = bs->drv;
>>> int ret, open_ret;
>>> int64_t len;
>>> if (!drv) {
>>> + error_setg(errp, "Block driver is closed");
>>> return -ENOMEDIUM;
>>> }
>>> len = bdrv_getlength(bs);
>>> if (len < 0) {
>>> + error_setg_errno(errp, -len, "Cannot get block device size");
>>> return len;
>>> }
>>> /* We should set all bits in all enabled dirty bitmaps, because dirty
>>> @@ -200,13 +203,18 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>> bdrv_set_dirty(bs, 0, len);
>>> if (drv->bdrv_snapshot_goto) {
>>> - return drv->bdrv_snapshot_goto(bs, snapshot_id);
>>> + ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
>>> + if (ret < 0) {
>>> + error_setg_errno(errp, -ret, "Failed to load snapshot");
>>> + }
>>> + return ret;
>>> }
>>> if (bs->file) {
>>> BlockDriverState *file;
>>> QDict *options = qdict_clone_shallow(bs->options);
>>> QDict *file_options;
>>> + Error *local_err = NULL;
>>> file = bs->file->bs;
>>> /* Prevent it from getting deleted when detached from bs */
>>> @@ -220,12 +228,14 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>> bdrv_unref_child(bs, bs->file);
>>> bs->file = NULL;
>>> - ret = bdrv_snapshot_goto(file, snapshot_id);
>>> - open_ret = drv->bdrv_open(bs, options, bs->open_flags, NULL);
>>> + ret = bdrv_snapshot_goto(file, snapshot_id, errp);
>>> + open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
>>> QDECREF(options);
>>> if (open_ret < 0) {
>>> bdrv_unref(file);
>>> bs->drv = NULL;
>>> + /* A bdrv_snapshot_goto() error takes precedence */
>> you return open_ret from bdrv_open and err msg from bdrv_snapshot_goto.
>> the may not correspond to each other.
> Hm, yes. Is this a problem, though?
>
> If it is, I guess we can change the return value below:
>
>>> + error_propagate(errp, local_err);
>>> return open_ret;
> return ret < 0 ? ret : open_ret;
>
> Would people prefer this?
looks good,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Kevin
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH for-2.11 2/3] block: Add errp to bdrv_all_goto_snapshot()
2017-11-20 14:50 [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps Kevin Wolf
2017-11-20 14:50 ` [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto() Kevin Wolf
@ 2017-11-20 14:50 ` Kevin Wolf
2017-11-20 16:14 ` Vladimir Sementsov-Ogievskiy
2017-11-20 14:50 ` [Qemu-devel] [PATCH for-2.11 3/3] block: Error out on load_vm with active dirty bitmaps Kevin Wolf
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2017-11-20 14:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, jsnow, vsementsov, den, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/snapshot.h | 3 ++-
block/snapshot.c | 11 ++++++-----
migration/savevm.c | 6 +++---
3 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index aeb80405e8..9407799941 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -84,7 +84,8 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
Error **err);
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
+ Error **errp);
int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
BlockDriverState *vm_state_bs,
diff --git a/block/snapshot.c b/block/snapshot.c
index 9c941e638d..13ec3b1c8c 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -467,9 +467,10 @@ fail:
}
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
+ Error **errp)
{
- int err = 0;
+ int ret = 0;
BlockDriverState *bs;
BdrvNextIterator it;
@@ -478,10 +479,10 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
aio_context_acquire(ctx);
if (bdrv_can_snapshot(bs)) {
- err = bdrv_snapshot_goto(bs, name, NULL);
+ ret = bdrv_snapshot_goto(bs, name, errp);
}
aio_context_release(ctx);
- if (err < 0) {
+ if (ret < 0) {
bdrv_next_cleanup(&it);
goto fail;
}
@@ -489,7 +490,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
fail:
*first_bad_bs = bs;
- return err;
+ return ret;
}
int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
diff --git a/migration/savevm.c b/migration/savevm.c
index 4a88228614..192f2d82cd 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2346,10 +2346,10 @@ int load_snapshot(const char *name, Error **errp)
/* Flush all IO requests so they don't interfere with the new state. */
bdrv_drain_all_begin();
- ret = bdrv_all_goto_snapshot(name, &bs);
+ ret = bdrv_all_goto_snapshot(name, &bs, errp);
if (ret < 0) {
- error_setg(errp, "Error %d while activating snapshot '%s' on '%s'",
- ret, name, bdrv_get_device_name(bs));
+ error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
+ name, bdrv_get_device_name(bs));
goto err_drain;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11 2/3] block: Add errp to bdrv_all_goto_snapshot()
2017-11-20 14:50 ` [Qemu-devel] [PATCH for-2.11 2/3] block: Add errp to bdrv_all_goto_snapshot() Kevin Wolf
@ 2017-11-20 16:14 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-20 16:14 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, eblake, jsnow, den, qemu-devel
20.11.2017 17:50, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/block/snapshot.h | 3 ++-
> block/snapshot.c | 11 ++++++-----
> migration/savevm.c | 6 +++---
> 3 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index aeb80405e8..9407799941 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -84,7 +84,8 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
> bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
> int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
> Error **err);
> -int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
> + Error **errp);
> int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
> int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
> BlockDriverState *vm_state_bs,
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 9c941e638d..13ec3b1c8c 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -467,9 +467,10 @@ fail:
> }
>
>
> -int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
> + Error **errp)
> {
> - int err = 0;
> + int ret = 0;
> BlockDriverState *bs;
> BdrvNextIterator it;
>
> @@ -478,10 +479,10 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>
> aio_context_acquire(ctx);
> if (bdrv_can_snapshot(bs)) {
> - err = bdrv_snapshot_goto(bs, name, NULL);
> + ret = bdrv_snapshot_goto(bs, name, errp);
> }
> aio_context_release(ctx);
> - if (err < 0) {
> + if (ret < 0) {
> bdrv_next_cleanup(&it);
> goto fail;
> }
> @@ -489,7 +490,7 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>
> fail:
> *first_bad_bs = bs;
> - return err;
> + return ret;
> }
>
> int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4a88228614..192f2d82cd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2346,10 +2346,10 @@ int load_snapshot(const char *name, Error **errp)
> /* Flush all IO requests so they don't interfere with the new state. */
> bdrv_drain_all_begin();
>
> - ret = bdrv_all_goto_snapshot(name, &bs);
> + ret = bdrv_all_goto_snapshot(name, &bs, errp);
> if (ret < 0) {
> - error_setg(errp, "Error %d while activating snapshot '%s' on '%s'",
> - ret, name, bdrv_get_device_name(bs));
> + error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
> + name, bdrv_get_device_name(bs));
possible resulting msg: "Could not load snapshot 'snap0' on 'dev0':
Failed to load snapshot". a bit weird. with this fixed or not:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> goto err_drain;
> }
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH for-2.11 3/3] block: Error out on load_vm with active dirty bitmaps
2017-11-20 14:50 [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps Kevin Wolf
2017-11-20 14:50 ` [Qemu-devel] [PATCH for-2.11 1/3] block: Add errp to bdrv_snapshot_goto() Kevin Wolf
2017-11-20 14:50 ` [Qemu-devel] [PATCH for-2.11 2/3] block: Add errp to bdrv_all_goto_snapshot() Kevin Wolf
@ 2017-11-20 14:50 ` Kevin Wolf
2017-11-20 16:16 ` Vladimir Sementsov-Ogievskiy
2017-11-20 15:14 ` [Qemu-devel] [PATCH for-2.11 0/3] " Eric Blake
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2017-11-20 14:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, jsnow, vsementsov, den, qemu-devel
Loading a snapshot invalidates the bitmap. Just marking all blocks dirty
is not a useful response in practice, instead the user needs to be aware
that we switch to a completely different state. If they are okay with
losing the dirty bitmap, they can just explicitly delete it.
This effectively reverts commit 04dec3c3ae5.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/snapshot.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 13ec3b1c8c..6b338978c5 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -182,25 +182,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
{
BlockDriver *drv = bs->drv;
int ret, open_ret;
- int64_t len;
if (!drv) {
error_setg(errp, "Block driver is closed");
return -ENOMEDIUM;
}
- len = bdrv_getlength(bs);
- if (len < 0) {
- error_setg_errno(errp, -len, "Cannot get block device size");
- return len;
+ if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
+ error_setg(errp, "Device has active dirty bitmaps");
+ return -EBUSY;
}
- /* We should set all bits in all enabled dirty bitmaps, because dirty
- * bitmaps reflect active state of disk and snapshot switch operation
- * actually dirties active state.
- * TODO: It may make sense not to set all bits but analyze block status of
- * current state and destination snapshot and do not set bits corresponding
- * to both-zero or both-unallocated areas. */
- bdrv_set_dirty(bs, 0, len);
if (drv->bdrv_snapshot_goto) {
ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
--
2.13.6
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11 3/3] block: Error out on load_vm with active dirty bitmaps
2017-11-20 14:50 ` [Qemu-devel] [PATCH for-2.11 3/3] block: Error out on load_vm with active dirty bitmaps Kevin Wolf
@ 2017-11-20 16:16 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-20 16:16 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, eblake, jsnow, den, qemu-devel
20.11.2017 17:50, Kevin Wolf wrote:
> Loading a snapshot invalidates the bitmap. Just marking all blocks dirty
> is not a useful response in practice, instead the user needs to be aware
> that we switch to a completely different state. If they are okay with
> losing the dirty bitmap, they can just explicitly delete it.
>
> This effectively reverts commit 04dec3c3ae5.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/snapshot.c | 15 +++------------
> 1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 13ec3b1c8c..6b338978c5 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -182,25 +182,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> {
> BlockDriver *drv = bs->drv;
> int ret, open_ret;
> - int64_t len;
>
> if (!drv) {
> error_setg(errp, "Block driver is closed");
> return -ENOMEDIUM;
> }
>
> - len = bdrv_getlength(bs);
> - if (len < 0) {
> - error_setg_errno(errp, -len, "Cannot get block device size");
> - return len;
> + if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
> + error_setg(errp, "Device has active dirty bitmaps");
> + return -EBUSY;
> }
> - /* We should set all bits in all enabled dirty bitmaps, because dirty
> - * bitmaps reflect active state of disk and snapshot switch operation
> - * actually dirties active state.
> - * TODO: It may make sense not to set all bits but analyze block status of
> - * current state and destination snapshot and do not set bits corresponding
> - * to both-zero or both-unallocated areas. */
> - bdrv_set_dirty(bs, 0, len);
>
> if (drv->bdrv_snapshot_goto) {
> ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps
2017-11-20 14:50 [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps Kevin Wolf
` (2 preceding siblings ...)
2017-11-20 14:50 ` [Qemu-devel] [PATCH for-2.11 3/3] block: Error out on load_vm with active dirty bitmaps Kevin Wolf
@ 2017-11-20 15:14 ` Eric Blake
2017-11-20 15:21 ` Denis V. Lunev
2017-11-21 0:37 ` John Snow
5 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2017-11-20 15:14 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, jsnow, vsementsov, den, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 860 bytes --]
On 11/20/2017 08:50 AM, Kevin Wolf wrote:
> Following the discussing at
> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg03572.html
> this implements the error return for loading a snapshot while dirty
> bitmaps are active.
>
> Kevin Wolf (3):
> block: Add errp to bdrv_snapshot_goto()
> block: Add errp to bdrv_all_goto_snapshot()
> block: Error out on load_vm with active dirty bitmaps
Reviewed-by: Eric Blake <eblake@redhat.com>
>
> include/block/snapshot.h | 6 ++++--
> block/snapshot.c | 43 +++++++++++++++++++++++--------------------
> migration/savevm.c | 6 +++---
> qemu-img.c | 6 +++---
> 4 files changed, 33 insertions(+), 28 deletions(-)
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps
2017-11-20 14:50 [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps Kevin Wolf
` (3 preceding siblings ...)
2017-11-20 15:14 ` [Qemu-devel] [PATCH for-2.11 0/3] " Eric Blake
@ 2017-11-20 15:21 ` Denis V. Lunev
2017-11-21 0:37 ` John Snow
5 siblings, 0 replies; 12+ messages in thread
From: Denis V. Lunev @ 2017-11-20 15:21 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, eblake, jsnow, vsementsov, qemu-devel
On 11/20/2017 05:50 PM, Kevin Wolf wrote:
> Following the discussing at
> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg03572.html
> this implements the error return for loading a snapshot while dirty
> bitmaps are active.
>
> Kevin Wolf (3):
> block: Add errp to bdrv_snapshot_goto()
> block: Add errp to bdrv_all_goto_snapshot()
> block: Error out on load_vm with active dirty bitmaps
>
> include/block/snapshot.h | 6 ++++--
> block/snapshot.c | 43 +++++++++++++++++++++++--------------------
> migration/savevm.c | 6 +++---
> qemu-img.c | 6 +++---
> 4 files changed, 33 insertions(+), 28 deletions(-)
>
Reviewed-by: Denis V. Lunev <den@openvz.org>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps
2017-11-20 14:50 [Qemu-devel] [PATCH for-2.11 0/3] block: Error out on load_vm with active dirty bitmaps Kevin Wolf
` (4 preceding siblings ...)
2017-11-20 15:21 ` Denis V. Lunev
@ 2017-11-21 0:37 ` John Snow
5 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2017-11-21 0:37 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: vsementsov, qemu-devel, mreitz, den
On 11/20/2017 09:50 AM, Kevin Wolf wrote:
> Following the discussing at
> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg03572.html
> this implements the error return for loading a snapshot while dirty
> bitmaps are active.
>
> Kevin Wolf (3):
> block: Add errp to bdrv_snapshot_goto()
> block: Add errp to bdrv_all_goto_snapshot()
> block: Error out on load_vm with active dirty bitmaps
>
> include/block/snapshot.h | 6 ++++--
> block/snapshot.c | 43 +++++++++++++++++++++++--------------------
> migration/savevm.c | 6 +++---
> qemu-img.c | 6 +++---
> 4 files changed, 33 insertions(+), 28 deletions(-)
>
You have enough reviews, but just so it's unambiguous, I'm fine with
this approach in lieu of Vladimir's; so:
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread