Message ID | 1386862440-8003-8-git-send-email-benoit@irqsave.net |
---|---|
State | New |
Headers | show |
On Thu, 12/12 16:34, Benoît Canet wrote: > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++------- > hmp.c | 4 +++- > qapi-schema.json | 13 ++++++++++--- > qmp-commands.hx | 11 ++++++++++- > 4 files changed, 71 insertions(+), 12 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 374d03d..1246544 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -940,14 +940,22 @@ static void blockdev_do_action(int kind, void *data, Error **errp) > qmp_transaction(&list, errp); > } > > -void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, > +void qmp_blockdev_snapshot_sync(bool has_device, const char *device, > + bool has_node_name, const char *node_name, > + const char *snapshot_file, > + bool has_snapshot_node_name, > + const char *snapshot_node_name, > bool has_format, const char *format, > - bool has_mode, enum NewImageMode mode, > - Error **errp) > + bool has_mode, NewImageMode mode, Error **errp) > { > BlockdevSnapshot snapshot = { > + .has_device = has_device, > .device = (char *) device, > + .has_node_name = has_node_name, > + .node_name = (char *) node_name, > .snapshot_file = (char *) snapshot_file, > + .has_snapshot_node_name = has_snapshot_node_name, > + .snapshot_node_name = (char *) snapshot_node_name, > .has_format = has_format, > .format = (char *) format, > .has_mode = has_mode, > @@ -1185,8 +1193,14 @@ static void external_snapshot_prepare(BlkTransactionState *common, > { > BlockDriver *drv; > int flags, ret; > + QDict *options = NULL; > Error *local_err = NULL; > + bool has_device = false; > const char *device; > + bool has_node_name = false; > + const char *node_name; > + bool has_snapshot_node_name = false; > + const char *snapshot_node_name; > const char *new_image_file; > const char *format = "qcow2"; > enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; > @@ -1197,7 +1211,14 @@ static void external_snapshot_prepare(BlkTransactionState *common, > /* get parameters */ > g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC); > > + has_device = action->blockdev_snapshot_sync->has_device; > device = action->blockdev_snapshot_sync->device; > + has_node_name = action->blockdev_snapshot_sync->has_node_name; > + node_name = action->blockdev_snapshot_sync->node_name; > + has_snapshot_node_name = > + action->blockdev_snapshot_sync->has_snapshot_node_name; > + snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name; > + > new_image_file = action->blockdev_snapshot_sync->snapshot_file; > if (action->blockdev_snapshot_sync->has_format) { > format = action->blockdev_snapshot_sync->format; > @@ -1213,9 +1234,21 @@ static void external_snapshot_prepare(BlkTransactionState *common, > return; > } > > - state->old_bs = bdrv_find(device); > - if (!state->old_bs) { > - error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + state->old_bs = bdrv_lookup_bs(has_device, device, > + has_node_name, node_name, > + &local_err); > + if (error_is_set(&local_err)) { > + error_propagate(errp, local_err); > + return; > + } > + > + if (has_node_name && !has_snapshot_node_name) { > + error_setg(errp, "New snapshot node name missing"); > + return; > + } > + > + if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) { > + error_setg(errp, "New snapshot node name already existing"); > return; > } > > @@ -1255,15 +1288,23 @@ static void external_snapshot_prepare(BlkTransactionState *common, > } > } > > + if (has_snapshot_node_name) { > + options = qdict_new(); > + qdict_put(options, "node-name", > + qstring_from_str(snapshot_node_name)); > + } > + > /* We will manually add the backing_hd field to the bs later */ > state->new_bs = bdrv_new(""); > /* TODO Inherit bs->options or only take explicit options with an > * extended QMP command? */ > - ret = bdrv_open(state->new_bs, new_image_file, NULL, > + ret = bdrv_open(state->new_bs, new_image_file, options, > flags | BDRV_O_NO_BACKING, drv, &local_err); > if (ret != 0) { > error_propagate(errp, local_err); > } > + > + QDECREF(options); > } > > static void external_snapshot_commit(BlkTransactionState *common) > diff --git a/hmp.c b/hmp.c > index 906ddb7..47dcf0c 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -971,7 +971,9 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict) > } > > mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS; > - qmp_blockdev_snapshot_sync(device, filename, !!format, format, > + qmp_blockdev_snapshot_sync(true, device, false, NULL, > + filename, false, NULL, > + !!format, format, > true, mode, &errp); > hmp_handle_error(mon, &errp); > } > diff --git a/qapi-schema.json b/qapi-schema.json > index 3977619..d7afb69 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1759,18 +1759,25 @@ > ## > # @BlockdevSnapshot > # > -# @device: the name of the device to generate the snapshot from. > +# Either @device or @node-name must be set but not both. > +# > +# @device: #optional the name of the device to generate the snapshot from. > +# > +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0) > # > # @snapshot-file: the target of the new image. A new file will be created. > # > +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0) > +# > # @format: #optional the format of the snapshot image, default is 'qcow2'. > # > # @mode: #optional whether and how QEMU should create a new image, default is > # 'absolute-paths'. > ## > { 'type': 'BlockdevSnapshot', > - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', > - '*mode': 'NewImageMode' } } > + 'data': { '*device': 'str', '*node-name': 'str', > + 'snapshot-file': 'str', '*snapshot-node-name': 'str', > + '*format': 'str', '*mode': 'NewImageMode' } } > > ## > # @BlockdevSnapshotInternal > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 5696b08..b62b0f5 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1038,7 +1038,9 @@ actions array: > - "data": a dictionary. The contents depend on the value > of "type". When "type" is "blockdev-snapshot-sync": > - "device": device name to snapshot (json-string) > + - "node-name": graph node name to snapshot (json-string) > - "snapshot-file": name of new image file (json-string) > + - "snapshot-node-name": graph node name of the new snapshot (json-string) > - "format": format of new image (json-string, optional) > - "mode": whether and how QEMU should create the snapshot file > (NewImageMode, optional, default "absolute-paths") > @@ -1053,6 +1055,11 @@ Example: > { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd0", > "snapshot-file": "/some/place/my-image", > "format": "qcow2" } }, > + { 'type': 'blockdev-snapshot-sync', 'data' : { "node-name": "myfile", > + "snapshot-file": "/some/place/my-image2", > + "snapshot-node-name": "node3432", > + "mode": "existing", > + "format": "qcow2" } }, > { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1", > "snapshot-file": "/some/place/my-image2", > "mode": "existing", > @@ -1066,7 +1073,7 @@ EQMP > > { > .name = "blockdev-snapshot-sync", > - .args_type = "device:B,snapshot-file:s,format:s?,mode:s?", > + .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?", > .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync, > }, > > @@ -1083,7 +1090,9 @@ snapshot image, default is qcow2. > Arguments: > > - "device": device name to snapshot (json-string) > +- "node-name": graph node name to snapshot (json-string) > - "snapshot-file": name of new image file (json-string) > +- "snapshot-node-name": graph node name of the new snapshot (json-string) > - "mode": whether and how QEMU should create the snapshot file > (NewImageMode, optional, default "absolute-paths") > - "format": format of new image (json-string, optional) > -- > 1.8.3.2 > > Reviewed-by: Fam Zheng <famz@redhat.com>
Am 12.12.2013 um 16:34 hat Benoît Canet geschrieben: > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++------- > hmp.c | 4 +++- > qapi-schema.json | 13 ++++++++++--- > qmp-commands.hx | 11 ++++++++++- > 4 files changed, 71 insertions(+), 12 deletions(-) > diff --git a/qapi-schema.json b/qapi-schema.json > index 3977619..d7afb69 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1759,18 +1759,25 @@ > ## > # @BlockdevSnapshot > # > -# @device: the name of the device to generate the snapshot from. > +# Either @device or @node-name must be set but not both. > +# > +# @device: #optional the name of the device to generate the snapshot from. > +# > +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0) > # > # @snapshot-file: the target of the new image. A new file will be created. > # > +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0) > +# I think we should document how this plays together with snapshot-file, format and mode. Perhaps this is actually different enough that it would justify creating a new QMP command 'blockdev-snapshot' that never creates image files but only ever takes existing nodes. The implementation of 'blockdev-snapshot- sync' could then become a wrapper around it. > # @format: #optional the format of the snapshot image, default is 'qcow2'. > # > # @mode: #optional whether and how QEMU should create a new image, default is > # 'absolute-paths'. > ## > { 'type': 'BlockdevSnapshot', > - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', > - '*mode': 'NewImageMode' } } > + 'data': { '*device': 'str', '*node-name': 'str', > + 'snapshot-file': 'str', '*snapshot-node-name': 'str', > + '*format': 'str', '*mode': 'NewImageMode' } } > > ## > # @BlockdevSnapshotInternal Kevin
Le Tuesday 21 Jan 2014 à 15:28:49 (+0100), Kevin Wolf a écrit : > Am 12.12.2013 um 16:34 hat Benoît Canet geschrieben: > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > --- > > blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++------- > > hmp.c | 4 +++- > > qapi-schema.json | 13 ++++++++++--- > > qmp-commands.hx | 11 ++++++++++- > > 4 files changed, 71 insertions(+), 12 deletions(-) > > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 3977619..d7afb69 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -1759,18 +1759,25 @@ > > ## > > # @BlockdevSnapshot > > # > > -# @device: the name of the device to generate the snapshot from. > > +# Either @device or @node-name must be set but not both. > > +# > > +# @device: #optional the name of the device to generate the snapshot from. > > +# > > +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0) > > # > > # @snapshot-file: the target of the new image. A new file will be created. > > # > > +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0) > > +# > > I think we should document how this plays together with snapshot-file, > format and mode. What kind of interactions do you expect ? The only kind of interaction I see is that setting node-name imply that the user really want to manipulate the graph and that snapshot-node-name is mandatory as a consequence. > > Perhaps this is actually different enough that it would justify creating > a new QMP command 'blockdev-snapshot' that never creates image files but > only ever takes existing nodes. The implementation of 'blockdev-snapshot- > sync' could then become a wrapper around it. What would be the benefits ? Best regards Benoît > > > # @format: #optional the format of the snapshot image, default is 'qcow2'. > > # > > # @mode: #optional whether and how QEMU should create a new image, default is > > # 'absolute-paths'. > > ## > > { 'type': 'BlockdevSnapshot', > > - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', > > - '*mode': 'NewImageMode' } } > > + 'data': { '*device': 'str', '*node-name': 'str', > > + 'snapshot-file': 'str', '*snapshot-node-name': 'str', > > + '*format': 'str', '*mode': 'NewImageMode' } } > > > > ## > > # @BlockdevSnapshotInternal > > Kevin
Am 22.01.2014 um 22:33 hat Benoît Canet geschrieben: > Le Tuesday 21 Jan 2014 à 15:28:49 (+0100), Kevin Wolf a écrit : > > Am 12.12.2013 um 16:34 hat Benoît Canet geschrieben: > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > > --- > > > blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++------- > > > hmp.c | 4 +++- > > > qapi-schema.json | 13 ++++++++++--- > > > qmp-commands.hx | 11 ++++++++++- > > > 4 files changed, 71 insertions(+), 12 deletions(-) > > > > > diff --git a/qapi-schema.json b/qapi-schema.json > > > index 3977619..d7afb69 100644 > > > --- a/qapi-schema.json > > > +++ b/qapi-schema.json > > > @@ -1759,18 +1759,25 @@ > > > ## > > > # @BlockdevSnapshot > > > # > > > -# @device: the name of the device to generate the snapshot from. > > > +# Either @device or @node-name must be set but not both. > > > +# > > > +# @device: #optional the name of the device to generate the snapshot from. > > > +# > > > +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0) > > > # > > > # @snapshot-file: the target of the new image. A new file will be created. > > > # > > > +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0) > > > +# > > > > I think we should document how this plays together with snapshot-file, > > format and mode. > > What kind of interactions do you expect ? > The only kind of interaction I see is that setting node-name imply that the > user really want to manipulate the graph and that snapshot-node-name is > mandatory as a consequence. Aha, I misunderstood. I thought you could pass the node name of an existing block device that should be used as the new overlay image. But in fact, you still create a new image and only assign the snapshot-node-name to the newly created node. Nothing wrong with the behaviour, it's just that device/node-name and snapshot-file/snapshot-node-name suggested a parallelity that isn't there. Perhaps someone has a good idea to improve the naming. Kevin
diff --git a/blockdev.c b/blockdev.c index 374d03d..1246544 100644 --- a/blockdev.c +++ b/blockdev.c @@ -940,14 +940,22 @@ static void blockdev_do_action(int kind, void *data, Error **errp) qmp_transaction(&list, errp); } -void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, +void qmp_blockdev_snapshot_sync(bool has_device, const char *device, + bool has_node_name, const char *node_name, + const char *snapshot_file, + bool has_snapshot_node_name, + const char *snapshot_node_name, bool has_format, const char *format, - bool has_mode, enum NewImageMode mode, - Error **errp) + bool has_mode, NewImageMode mode, Error **errp) { BlockdevSnapshot snapshot = { + .has_device = has_device, .device = (char *) device, + .has_node_name = has_node_name, + .node_name = (char *) node_name, .snapshot_file = (char *) snapshot_file, + .has_snapshot_node_name = has_snapshot_node_name, + .snapshot_node_name = (char *) snapshot_node_name, .has_format = has_format, .format = (char *) format, .has_mode = has_mode, @@ -1185,8 +1193,14 @@ static void external_snapshot_prepare(BlkTransactionState *common, { BlockDriver *drv; int flags, ret; + QDict *options = NULL; Error *local_err = NULL; + bool has_device = false; const char *device; + bool has_node_name = false; + const char *node_name; + bool has_snapshot_node_name = false; + const char *snapshot_node_name; const char *new_image_file; const char *format = "qcow2"; enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; @@ -1197,7 +1211,14 @@ static void external_snapshot_prepare(BlkTransactionState *common, /* get parameters */ g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC); + has_device = action->blockdev_snapshot_sync->has_device; device = action->blockdev_snapshot_sync->device; + has_node_name = action->blockdev_snapshot_sync->has_node_name; + node_name = action->blockdev_snapshot_sync->node_name; + has_snapshot_node_name = + action->blockdev_snapshot_sync->has_snapshot_node_name; + snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name; + new_image_file = action->blockdev_snapshot_sync->snapshot_file; if (action->blockdev_snapshot_sync->has_format) { format = action->blockdev_snapshot_sync->format; @@ -1213,9 +1234,21 @@ static void external_snapshot_prepare(BlkTransactionState *common, return; } - state->old_bs = bdrv_find(device); - if (!state->old_bs) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device); + state->old_bs = bdrv_lookup_bs(has_device, device, + has_node_name, node_name, + &local_err); + if (error_is_set(&local_err)) { + error_propagate(errp, local_err); + return; + } + + if (has_node_name && !has_snapshot_node_name) { + error_setg(errp, "New snapshot node name missing"); + return; + } + + if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) { + error_setg(errp, "New snapshot node name already existing"); return; } @@ -1255,15 +1288,23 @@ static void external_snapshot_prepare(BlkTransactionState *common, } } + if (has_snapshot_node_name) { + options = qdict_new(); + qdict_put(options, "node-name", + qstring_from_str(snapshot_node_name)); + } + /* We will manually add the backing_hd field to the bs later */ state->new_bs = bdrv_new(""); /* TODO Inherit bs->options or only take explicit options with an * extended QMP command? */ - ret = bdrv_open(state->new_bs, new_image_file, NULL, + ret = bdrv_open(state->new_bs, new_image_file, options, flags | BDRV_O_NO_BACKING, drv, &local_err); if (ret != 0) { error_propagate(errp, local_err); } + + QDECREF(options); } static void external_snapshot_commit(BlkTransactionState *common) diff --git a/hmp.c b/hmp.c index 906ddb7..47dcf0c 100644 --- a/hmp.c +++ b/hmp.c @@ -971,7 +971,9 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict) } mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS; - qmp_blockdev_snapshot_sync(device, filename, !!format, format, + qmp_blockdev_snapshot_sync(true, device, false, NULL, + filename, false, NULL, + !!format, format, true, mode, &errp); hmp_handle_error(mon, &errp); } diff --git a/qapi-schema.json b/qapi-schema.json index 3977619..d7afb69 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1759,18 +1759,25 @@ ## # @BlockdevSnapshot # -# @device: the name of the device to generate the snapshot from. +# Either @device or @node-name must be set but not both. +# +# @device: #optional the name of the device to generate the snapshot from. +# +# @node-name: #optional graph node name to generate the snapshot from (Since 2.0) # # @snapshot-file: the target of the new image. A new file will be created. # +# @snapshot-node-name: #optional the graph node name of the new image (Since 2.0) +# # @format: #optional the format of the snapshot image, default is 'qcow2'. # # @mode: #optional whether and how QEMU should create a new image, default is # 'absolute-paths'. ## { 'type': 'BlockdevSnapshot', - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', - '*mode': 'NewImageMode' } } + 'data': { '*device': 'str', '*node-name': 'str', + 'snapshot-file': 'str', '*snapshot-node-name': 'str', + '*format': 'str', '*mode': 'NewImageMode' } } ## # @BlockdevSnapshotInternal diff --git a/qmp-commands.hx b/qmp-commands.hx index 5696b08..b62b0f5 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1038,7 +1038,9 @@ actions array: - "data": a dictionary. The contents depend on the value of "type". When "type" is "blockdev-snapshot-sync": - "device": device name to snapshot (json-string) + - "node-name": graph node name to snapshot (json-string) - "snapshot-file": name of new image file (json-string) + - "snapshot-node-name": graph node name of the new snapshot (json-string) - "format": format of new image (json-string, optional) - "mode": whether and how QEMU should create the snapshot file (NewImageMode, optional, default "absolute-paths") @@ -1053,6 +1055,11 @@ Example: { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd0", "snapshot-file": "/some/place/my-image", "format": "qcow2" } }, + { 'type': 'blockdev-snapshot-sync', 'data' : { "node-name": "myfile", + "snapshot-file": "/some/place/my-image2", + "snapshot-node-name": "node3432", + "mode": "existing", + "format": "qcow2" } }, { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1", "snapshot-file": "/some/place/my-image2", "mode": "existing", @@ -1066,7 +1073,7 @@ EQMP { .name = "blockdev-snapshot-sync", - .args_type = "device:B,snapshot-file:s,format:s?,mode:s?", + .args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?", .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync, }, @@ -1083,7 +1090,9 @@ snapshot image, default is qcow2. Arguments: - "device": device name to snapshot (json-string) +- "node-name": graph node name to snapshot (json-string) - "snapshot-file": name of new image file (json-string) +- "snapshot-node-name": graph node name of the new snapshot (json-string) - "mode": whether and how QEMU should create the snapshot file (NewImageMode, optional, default "absolute-paths") - "format": format of new image (json-string, optional)
Signed-off-by: Benoit Canet <benoit@irqsave.net> --- blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++------- hmp.c | 4 +++- qapi-schema.json | 13 ++++++++++--- qmp-commands.hx | 11 ++++++++++- 4 files changed, 71 insertions(+), 12 deletions(-)