| Message ID | 1373521624-4380-4-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben: > Unlike savevm, the qmp_transaction interface will not generate > snapshot name automatically, saving trouble to return information > of the new created snapshot. > > Although qcow2 support storing multiple snapshots with same name > but different ID, here it will fail when an snapshot with that name > already exist before the operation. Format such as rbd do not support > ID at all, and in most case, it means trouble to user when he faces > multiple snapshots with same name, so ban that case. Request with > empty name will be rejected. > > Snapshot ID can't be specified in this interface. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qapi-schema.json | 18 ++++++++- > qmp-commands.hx | 34 ++++++++++++---- > 3 files changed, 160 insertions(+), 9 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index b3a57e0..6554768 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -808,6 +808,118 @@ struct BlkTransactionState { > QSIMPLEQ_ENTRY(BlkTransactionState) entry; > }; > > +/* internal snapshot private data */ > +typedef struct InternalSnapshotState { > + BlkTransactionState common; > + BlockDriverState *bs; > + QEMUSnapshotInfo sn; > +} InternalSnapshotState; > + > +static void internal_snapshot_prepare(BlkTransactionState *common, > + Error **errp) > +{ > + const char *device; > + const char *name; > + BlockDriverState *bs; > + QEMUSnapshotInfo sn, *sn1; > + bool ret; > + qemu_timeval tv; > + BlockdevSnapshotInternal *internal; > + InternalSnapshotState *state; > + int ret1; > + > + g_assert(common->action->kind == > + TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC); > + internal = common->action->blockdev_snapshot_internal_sync; > + state = DO_UPCAST(InternalSnapshotState, common, common); > + > + /* 1. parse input */ > + device = internal->device; > + name = internal->name; > + > + /* 2. check for validation */ > + bs = bdrv_find(device); > + if (!bs) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + return; > + } > + > + if (!bdrv_is_inserted(bs)) { > + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); > + return; > + } > + > + if (bdrv_is_read_only(bs)) { > + error_set(errp, QERR_DEVICE_IS_READ_ONLY, device); > + return; > + } > + > + if (!bdrv_can_snapshot(bs)) { > + error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > + bs->drv->format_name, device, "internal snapshot"); > + return; > + } > + > + if (!strlen(name)) { > + error_setg(errp, "Name is empty on device '%s'", device); Name is empty. This has nothing to do with the device. > + return; > + } > + > + /* check whether a snapshot with name exist */ > + ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp); > + if (error_is_set(errp)) { > + return; > + } > + if (ret) { Save one line with '} else if {' ? > + error_setg(errp, > + "Snapshot with name '%s' already exists on device '%s'", > + name, device); > + return; > + } > + > + /* 3. take the snapshot */ > + sn1 = &state->sn; do_savevm() has a memset() to clear all of sn1 before it starts filling it in. Should we do the same here? For example sn1.vm_state_size looks undefined. It also stops the VM before saving the snapshot. I don't think this is necessary here because we don't save the VM state, but do we need at least the bdrv_flush/bdrv_drain_all part of it? > + pstrcpy(sn1->name, sizeof(sn1->name), name); > + qemu_gettimeofday(&tv); > + sn1->date_sec = tv.tv_sec; > + sn1->date_nsec = tv.tv_usec * 1000; > + sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock); > + > + ret1 = bdrv_snapshot_create(bs, sn1); > + if (ret1 < 0) { > + error_setg_errno(errp, -ret1, > + "Failed to create snapshot '%s' on device '%s'", > + name, device); > + return; > + } > + > + /* 4. succeed, mark a snapshot is created */ > + state->bs = bs; > +} > + > +static void internal_snapshot_abort(BlkTransactionState *common) > +{ > + InternalSnapshotState *state = > + DO_UPCAST(InternalSnapshotState, common, common); > + BlockDriverState *bs = state->bs; > + QEMUSnapshotInfo *sn = &state->sn; > + Error *local_error = NULL; > + > + if (!bs) { > + return; > + } > + > + if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) { > + error_report("Failed to delete snapshot with id '%s' and name '%s' on " > + "device '%s' in abort, reason is: '%s'", > + sn->id_str, > + sn->name, > + bdrv_get_device_name(bs), > + error_get_pretty(local_error)); > + error_free(local_error); See, here you're doing the right thing if bdrv_snapshot_delete() returns simple errors like "Failed to remove from snapshot list". With the changes the earlier patch made to qcow2, you end up with this, though: Failed to delete snapshot with id 'uninitialised' and name 'test' on device 'ide0-hd0' in abort, reason is: 'Failed to remove snapshot with ID 'uninitialised' and name 'test' from the snapshot list on device 'ide0-hd0'' We need to standardise on the minimal error information that makes the error unambiguous in order to avoid such duplication. To sum up: Leave this code as it is, but change qcow2 etc. to remove ID, name and device from their messages. > + } > +} Kevin
于 2013-7-18 20:22, Kevin Wolf 写道: > Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben: >> Unlike savevm, the qmp_transaction interface will not generate >> snapshot name automatically, saving trouble to return information >> of the new created snapshot. >> >> Although qcow2 support storing multiple snapshots with same name >> but different ID, here it will fail when an snapshot with that name >> already exist before the operation. Format such as rbd do not support >> ID at all, and in most case, it means trouble to user when he faces >> multiple snapshots with same name, so ban that case. Request with >> empty name will be rejected. >> >> Snapshot ID can't be specified in this interface. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> --- >> blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qapi-schema.json | 18 ++++++++- >> qmp-commands.hx | 34 ++++++++++++---- >> 3 files changed, 160 insertions(+), 9 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index b3a57e0..6554768 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -808,6 +808,118 @@ struct BlkTransactionState { >> QSIMPLEQ_ENTRY(BlkTransactionState) entry; >> }; >> >> +/* internal snapshot private data */ >> +typedef struct InternalSnapshotState { >> + BlkTransactionState common; >> + BlockDriverState *bs; >> + QEMUSnapshotInfo sn; >> +} InternalSnapshotState; >> + >> +static void internal_snapshot_prepare(BlkTransactionState *common, >> + Error **errp) >> +{ >> + const char *device; >> + const char *name; >> + BlockDriverState *bs; >> + QEMUSnapshotInfo sn, *sn1; >> + bool ret; >> + qemu_timeval tv; >> + BlockdevSnapshotInternal *internal; >> + InternalSnapshotState *state; >> + int ret1; >> + >> + g_assert(common->action->kind == >> + TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC); >> + internal = common->action->blockdev_snapshot_internal_sync; >> + state = DO_UPCAST(InternalSnapshotState, common, common); >> + >> + /* 1. parse input */ >> + device = internal->device; >> + name = internal->name; >> + >> + /* 2. check for validation */ >> + bs = bdrv_find(device); >> + if (!bs) { >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> + } >> + >> + if (!bdrv_is_inserted(bs)) { >> + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); >> + return; >> + } >> + >> + if (bdrv_is_read_only(bs)) { >> + error_set(errp, QERR_DEVICE_IS_READ_ONLY, device); >> + return; >> + } >> + >> + if (!bdrv_can_snapshot(bs)) { >> + error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, >> + bs->drv->format_name, device, "internal snapshot"); >> + return; >> + } >> + >> + if (!strlen(name)) { >> + error_setg(errp, "Name is empty on device '%s'", device); > > Name is empty. This has nothing to do with the device. > OK. >> + return; >> + } >> + >> + /* check whether a snapshot with name exist */ >> + ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp); >> + if (error_is_set(errp)) { >> + return; >> + } >> + if (ret) { > > Save one line with '} else if {' ? > will change, thanks. >> + error_setg(errp, >> + "Snapshot with name '%s' already exists on device '%s'", >> + name, device); >> + return; >> + } >> + >> + /* 3. take the snapshot */ >> + sn1 = &state->sn; > > do_savevm() has a memset() to clear all of sn1 before it starts filling > it in. Should we do the same here? For example sn1.vm_state_size looks > undefined. > I think state->sn is set to zero by qmp_transaction(): state = g_malloc0(ops->instance_size); > It also stops the VM before saving the snapshot. I don't think this is > necessary here because we don't save the VM state, but do we need at > least the bdrv_flush/bdrv_drain_all part of it? > bdrv_drain_all() is called already in qmp_transaction(). But I plan add an explicit API later for bdrv_flush/bdrv_drain, which ensure data is going to underlining storage. >> + pstrcpy(sn1->name, sizeof(sn1->name), name); >> + qemu_gettimeofday(&tv); >> + sn1->date_sec = tv.tv_sec; >> + sn1->date_nsec = tv.tv_usec * 1000; >> + sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock); >> + >> + ret1 = bdrv_snapshot_create(bs, sn1); >> + if (ret1 < 0) { >> + error_setg_errno(errp, -ret1, >> + "Failed to create snapshot '%s' on device '%s'", >> + name, device); >> + return; >> + } >> + >> + /* 4. succeed, mark a snapshot is created */ >> + state->bs = bs; >> +} >> + >> +static void internal_snapshot_abort(BlkTransactionState *common) >> +{ >> + InternalSnapshotState *state = >> + DO_UPCAST(InternalSnapshotState, common, common); >> + BlockDriverState *bs = state->bs; >> + QEMUSnapshotInfo *sn = &state->sn; >> + Error *local_error = NULL; >> + >> + if (!bs) { >> + return; >> + } >> + >> + if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) { >> + error_report("Failed to delete snapshot with id '%s' and name '%s' on " >> + "device '%s' in abort, reason is: '%s'", >> + sn->id_str, >> + sn->name, >> + bdrv_get_device_name(bs), >> + error_get_pretty(local_error)); >> + error_free(local_error); > > See, here you're doing the right thing if bdrv_snapshot_delete() returns > simple errors like "Failed to remove from snapshot list". With the > changes the earlier patch made to qcow2, you end up with this, though: > > Failed to delete snapshot with id 'uninitialised' and name 'test' on > device 'ide0-hd0' in abort, reason is: 'Failed to remove snapshot > with ID 'uninitialised' and name 'test' from the snapshot list on > device 'ide0-hd0'' > > We need to standardise on the minimal error information that makes the > error unambiguous in order to avoid such duplication. > > To sum up: Leave this code as it is, but change qcow2 etc. to remove ID, > name and device from their messages. > OK, will remove them in the implementions. >> + } >> +} > > Kevin >
Am 19.07.2013 um 11:19 hat Wenchao Xia geschrieben: > 于 2013-7-18 20:22, Kevin Wolf 写道: > >Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben: > >>Unlike savevm, the qmp_transaction interface will not generate > >>snapshot name automatically, saving trouble to return information > >>of the new created snapshot. > >> > >>Although qcow2 support storing multiple snapshots with same name > >>but different ID, here it will fail when an snapshot with that name > >>already exist before the operation. Format such as rbd do not support > >>ID at all, and in most case, it means trouble to user when he faces > >>multiple snapshots with same name, so ban that case. Request with > >>empty name will be rejected. > >> > >>Snapshot ID can't be specified in this interface. > >> > >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > >>--- > >> blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> qapi-schema.json | 18 ++++++++- > >> qmp-commands.hx | 34 ++++++++++++---- > >> 3 files changed, 160 insertions(+), 9 deletions(-) > >> > >>diff --git a/blockdev.c b/blockdev.c > >>index b3a57e0..6554768 100644 > >>--- a/blockdev.c > >>+++ b/blockdev.c > >>@@ -808,6 +808,118 @@ struct BlkTransactionState { > >> QSIMPLEQ_ENTRY(BlkTransactionState) entry; > >> }; > >> > >>+/* internal snapshot private data */ > >>+typedef struct InternalSnapshotState { > >>+ BlkTransactionState common; > >>+ BlockDriverState *bs; > >>+ QEMUSnapshotInfo sn; > >>+} InternalSnapshotState; > >>+ > >>+static void internal_snapshot_prepare(BlkTransactionState *common, > >>+ Error **errp) > >>+{ > >>+ const char *device; > >>+ const char *name; > >>+ BlockDriverState *bs; > >>+ QEMUSnapshotInfo sn, *sn1; > >>+ bool ret; > >>+ qemu_timeval tv; > >>+ BlockdevSnapshotInternal *internal; > >>+ InternalSnapshotState *state; > >>+ int ret1; > >>+ > >>+ g_assert(common->action->kind == > >>+ TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC); > >>+ internal = common->action->blockdev_snapshot_internal_sync; > >>+ state = DO_UPCAST(InternalSnapshotState, common, common); > >>+ > >>+ /* 1. parse input */ > >>+ device = internal->device; > >>+ name = internal->name; > >>+ > >>+ /* 2. check for validation */ > >>+ bs = bdrv_find(device); > >>+ if (!bs) { > >>+ error_set(errp, QERR_DEVICE_NOT_FOUND, device); > >>+ return; > >>+ } > >>+ > >>+ if (!bdrv_is_inserted(bs)) { > >>+ error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); > >>+ return; > >>+ } > >>+ > >>+ if (bdrv_is_read_only(bs)) { > >>+ error_set(errp, QERR_DEVICE_IS_READ_ONLY, device); > >>+ return; > >>+ } > >>+ > >>+ if (!bdrv_can_snapshot(bs)) { > >>+ error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > >>+ bs->drv->format_name, device, "internal snapshot"); > >>+ return; > >>+ } > >>+ > >>+ if (!strlen(name)) { > >>+ error_setg(errp, "Name is empty on device '%s'", device); > > > >Name is empty. This has nothing to do with the device. > > > OK. > > >>+ return; > >>+ } > >>+ > >>+ /* check whether a snapshot with name exist */ > >>+ ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp); > >>+ if (error_is_set(errp)) { > >>+ return; > >>+ } > >>+ if (ret) { > > > >Save one line with '} else if {' ? > > > will change, thanks. > > >>+ error_setg(errp, > >>+ "Snapshot with name '%s' already exists on device '%s'", > >>+ name, device); > >>+ return; > >>+ } > >>+ > >>+ /* 3. take the snapshot */ > >>+ sn1 = &state->sn; > > > >do_savevm() has a memset() to clear all of sn1 before it starts filling > >it in. Should we do the same here? For example sn1.vm_state_size looks > >undefined. > > > I think state->sn is set to zero by > qmp_transaction(): > state = g_malloc0(ops->instance_size); Oh, yes. I was confused by the fact that there is a local sn, which isn't related to sn1 at all. Perhaps some renaming to make things clearer? Kevin
于 2013-7-19 18:13, Kevin Wolf 写道: > Am 19.07.2013 um 11:19 hat Wenchao Xia geschrieben: >> 于 2013-7-18 20:22, Kevin Wolf 写道: >>> Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben: >>>> Unlike savevm, the qmp_transaction interface will not generate >>>> snapshot name automatically, saving trouble to return information >>>> of the new created snapshot. >>>> >>>> Although qcow2 support storing multiple snapshots with same name >>>> but different ID, here it will fail when an snapshot with that name >>>> already exist before the operation. Format such as rbd do not support >>>> ID at all, and in most case, it means trouble to user when he faces >>>> multiple snapshots with same name, so ban that case. Request with >>>> empty name will be rejected. >>>> >>>> Snapshot ID can't be specified in this interface. >>>> >>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >>>> --- >>>> blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> qapi-schema.json | 18 ++++++++- >>>> qmp-commands.hx | 34 ++++++++++++---- >>>> 3 files changed, 160 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/blockdev.c b/blockdev.c >>>> index b3a57e0..6554768 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -808,6 +808,118 @@ struct BlkTransactionState { >>>> QSIMPLEQ_ENTRY(BlkTransactionState) entry; >>>> }; >>>> >>>> +/* internal snapshot private data */ >>>> +typedef struct InternalSnapshotState { >>>> + BlkTransactionState common; >>>> + BlockDriverState *bs; >>>> + QEMUSnapshotInfo sn; >>>> +} InternalSnapshotState; >>>> + >>>> +static void internal_snapshot_prepare(BlkTransactionState *common, >>>> + Error **errp) >>>> +{ >>>> + const char *device; >>>> + const char *name; >>>> + BlockDriverState *bs; >>>> + QEMUSnapshotInfo sn, *sn1; >>>> + bool ret; >>>> + qemu_timeval tv; >>>> + BlockdevSnapshotInternal *internal; >>>> + InternalSnapshotState *state; >>>> + int ret1; >>>> + >>>> + g_assert(common->action->kind == >>>> + TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC); >>>> + internal = common->action->blockdev_snapshot_internal_sync; >>>> + state = DO_UPCAST(InternalSnapshotState, common, common); >>>> + >>>> + /* 1. parse input */ >>>> + device = internal->device; >>>> + name = internal->name; >>>> + >>>> + /* 2. check for validation */ >>>> + bs = bdrv_find(device); >>>> + if (!bs) { >>>> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >>>> + return; >>>> + } >>>> + >>>> + if (!bdrv_is_inserted(bs)) { >>>> + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); >>>> + return; >>>> + } >>>> + >>>> + if (bdrv_is_read_only(bs)) { >>>> + error_set(errp, QERR_DEVICE_IS_READ_ONLY, device); >>>> + return; >>>> + } >>>> + >>>> + if (!bdrv_can_snapshot(bs)) { >>>> + error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, >>>> + bs->drv->format_name, device, "internal snapshot"); >>>> + return; >>>> + } >>>> + >>>> + if (!strlen(name)) { >>>> + error_setg(errp, "Name is empty on device '%s'", device); >>> >>> Name is empty. This has nothing to do with the device. >>> >> OK. >> >>>> + return; >>>> + } >>>> + >>>> + /* check whether a snapshot with name exist */ >>>> + ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp); >>>> + if (error_is_set(errp)) { >>>> + return; >>>> + } >>>> + if (ret) { >>> >>> Save one line with '} else if {' ? >>> >> will change, thanks. >> >>>> + error_setg(errp, >>>> + "Snapshot with name '%s' already exists on device '%s'", >>>> + name, device); >>>> + return; >>>> + } >>>> + >>>> + /* 3. take the snapshot */ >>>> + sn1 = &state->sn; >>> >>> do_savevm() has a memset() to clear all of sn1 before it starts filling >>> it in. Should we do the same here? For example sn1.vm_state_size looks >>> undefined. >>> >> I think state->sn is set to zero by >> qmp_transaction(): >> state = g_malloc0(ops->instance_size); > > Oh, yes. I was confused by the fact that there is a local sn, which > isn't related to sn1 at all. Perhaps some renaming to make things > clearer? > > Kevin > sure, will rename local sn to sn_old.
diff --git a/blockdev.c b/blockdev.c index b3a57e0..6554768 100644 --- a/blockdev.c +++ b/blockdev.c @@ -808,6 +808,118 @@ struct BlkTransactionState { QSIMPLEQ_ENTRY(BlkTransactionState) entry; }; +/* internal snapshot private data */ +typedef struct InternalSnapshotState { + BlkTransactionState common; + BlockDriverState *bs; + QEMUSnapshotInfo sn; +} InternalSnapshotState; + +static void internal_snapshot_prepare(BlkTransactionState *common, + Error **errp) +{ + const char *device; + const char *name; + BlockDriverState *bs; + QEMUSnapshotInfo sn, *sn1; + bool ret; + qemu_timeval tv; + BlockdevSnapshotInternal *internal; + InternalSnapshotState *state; + int ret1; + + g_assert(common->action->kind == + TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC); + internal = common->action->blockdev_snapshot_internal_sync; + state = DO_UPCAST(InternalSnapshotState, common, common); + + /* 1. parse input */ + device = internal->device; + name = internal->name; + + /* 2. check for validation */ + bs = bdrv_find(device); + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; + } + + if (!bdrv_is_inserted(bs)) { + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); + return; + } + + if (bdrv_is_read_only(bs)) { + error_set(errp, QERR_DEVICE_IS_READ_ONLY, device); + return; + } + + if (!bdrv_can_snapshot(bs)) { + error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, + bs->drv->format_name, device, "internal snapshot"); + return; + } + + if (!strlen(name)) { + error_setg(errp, "Name is empty on device '%s'", device); + return; + } + + /* check whether a snapshot with name exist */ + ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp); + if (error_is_set(errp)) { + return; + } + if (ret) { + error_setg(errp, + "Snapshot with name '%s' already exists on device '%s'", + name, device); + return; + } + + /* 3. take the snapshot */ + sn1 = &state->sn; + pstrcpy(sn1->name, sizeof(sn1->name), name); + qemu_gettimeofday(&tv); + sn1->date_sec = tv.tv_sec; + sn1->date_nsec = tv.tv_usec * 1000; + sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock); + + ret1 = bdrv_snapshot_create(bs, sn1); + if (ret1 < 0) { + error_setg_errno(errp, -ret1, + "Failed to create snapshot '%s' on device '%s'", + name, device); + return; + } + + /* 4. succeed, mark a snapshot is created */ + state->bs = bs; +} + +static void internal_snapshot_abort(BlkTransactionState *common) +{ + InternalSnapshotState *state = + DO_UPCAST(InternalSnapshotState, common, common); + BlockDriverState *bs = state->bs; + QEMUSnapshotInfo *sn = &state->sn; + Error *local_error = NULL; + + if (!bs) { + return; + } + + if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) { + error_report("Failed to delete snapshot with id '%s' and name '%s' on " + "device '%s' in abort, reason is: '%s'", + sn->id_str, + sn->name, + bdrv_get_device_name(bs), + error_get_pretty(local_error)); + error_free(local_error); + } +} + /* external snapshot private data */ typedef struct ExternalSnapshotState { BlkTransactionState common; @@ -990,6 +1102,11 @@ static const BdrvActionOps actions[] = { .prepare = abort_prepare, .commit = abort_commit, }, + [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = { + .instance_size = sizeof(InternalSnapshotState), + .prepare = internal_snapshot_prepare, + .abort = internal_snapshot_abort, + }, }; /* diff --git a/qapi-schema.json b/qapi-schema.json index b251d28..5ec55d6 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1621,6 +1621,21 @@ { 'type': 'BlockdevSnapshot', 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', '*mode': 'NewImageMode' } } +## +# @BlockdevSnapshotInternal +# +# @device: the name of the device to generate the snapshot from +# +# @name: the name of the internal snapshot to be created +# +# Notes: In transaction, if @name is empty, or any snapshot matching @name +# exists, the operation will fail. Only some image formats support it, +# for example, qcow2, rbd, and sheepdog. +# +# Since: 1.6 +## +{ 'type': 'BlockdevSnapshotInternal', + 'data': { 'device': 'str', 'name': 'str' } } ## # @DriveBackup @@ -1679,7 +1694,8 @@ 'data': { 'blockdev-snapshot-sync': 'BlockdevSnapshot', 'drive-backup': 'DriveBackup', - 'abort': 'Abort' + 'abort': 'Abort', + 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal' } } ## diff --git a/qmp-commands.hx b/qmp-commands.hx index 362f0e1..0277f7e 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -993,14 +993,15 @@ SQMP transaction ----------- -Atomically operate on one or more block devices. The only supported -operation for now is snapshotting. If there is any failure performing -any of the operations, all snapshots for the group are abandoned, and -the original disks pre-snapshot attempt are used. +Atomically operate on one or more block devices. The only supported operations +for now are drive-backup, internal and external snapshotting. A list of +dictionaries is accepted, that contains the actions to be performed. +If there is any failure performing any of the operations, all operations +for the group are abandoned. -A list of dictionaries is accepted, that contains the actions to be performed. -For snapshots this is the device, the file to use for the new snapshot, -and the format. The default format, if not specified, is qcow2. +For external snapshots, the dictionary contains the device, the file to use for +the new snapshot, and the format. The default format, if not specified, is +qcow2. Each new snapshot defaults to being created by QEMU (wiping any contents if the file already exists), but it is also possible to reuse @@ -1009,6 +1010,17 @@ the new image file has the same contents as the current one; QEMU cannot perform any meaningful check. Typically this is achieved by using the current image file as the backing file for the new image. +On failure, the original disks pre-snapshot attempt will be used. + +For internal snapshots, the dictionary contains the device and the snapshot's +name. If an internal snapshot matching name already exists, the request will +be rejected. Only some image formats support it, for example, qcow2, rbd, +and sheepdog. + +On failure, qemu will try delete the newly created internal snapshot in the +transaction. When an I/O error occurs during deletion, the user needs to fix +it later with qemu-img or other command. + Arguments: actions array: @@ -1021,6 +1033,9 @@ actions array: - "format": format of new image (json-string, optional) - "mode": whether and how QEMU should create the snapshot file (NewImageMode, optional, default "absolute-paths") + When "type" is "blockdev-snapshot-internal-sync": + - "device": device name to snapshot (json-string) + - "name": name of the new snapshot (json-string) Example: @@ -1032,7 +1047,10 @@ Example: { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1", "snapshot-file": "/some/place/my-image2", "mode": "existing", - "format": "qcow2" } } ] } } + "format": "qcow2" } }, + { 'type': 'blockdev-snapshot-internal-sync', 'data' : { + "device": "ide-hd2", + "name": "snapshot0" } } ] } } <- { "return": {} } EQMP
Unlike savevm, the qmp_transaction interface will not generate snapshot name automatically, saving trouble to return information of the new created snapshot. Although qcow2 support storing multiple snapshots with same name but different ID, here it will fail when an snapshot with that name already exist before the operation. Format such as rbd do not support ID at all, and in most case, it means trouble to user when he faces multiple snapshots with same name, so ban that case. Request with empty name will be rejected. Snapshot ID can't be specified in this interface. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- blockdev.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi-schema.json | 18 ++++++++- qmp-commands.hx | 34 ++++++++++++---- 3 files changed, 160 insertions(+), 9 deletions(-)