| Message ID | 1422387983-32153-42-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/27/2015 12:46 PM, Max Reitz wrote: > And a helper function for that which directly takes a pointer to the BDS > to be inserted instead of its node-name (which will be used for > implementing 'change' using blockdev-insert-medium). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > blockdev.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 17 +++++++++++++++++ > qmp-commands.hx | 38 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 98 insertions(+) > > +static void qmp_blockdev_insert_anon_medium(const char *device, > + BlockDriverState *bs, Error **errp) > +{ > + BlockBackend *blk; > + > + blk = blk_by_name(device); > + if (!blk) { > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > + return; > + } > + > + if (!blk_dev_has_removable_media(blk)) { > + error_setg(errp, "Device '%s' is not removable", device); > + return; > + } > + > + if (!blk_dev_is_tray_open(blk)) { > + error_setg(errp, "Tray of device '%s' is not open", device); > + return; > + } > + > + if (blk_bs(blk)) { > + error_setg(errp, "There already is a medium in device '%s'", device); > + return; > + } Good, you didn't implement hot-swap semantics of replacing an existing medium (that gets too confusing; so I _like_ that you force the user to consider all the steps through multiple low-level commands). > + > +Example (1): I'll quit pointing it out; but if you clean up the useless (1) in one patch, do it across the series. > + > +-> { "execute": "blockdev-add", > + "arguments": { "options": { "id": "backend0", > + "node-name": "node0", Why is 'id' needed? Isn't the point of this command sequence to create a BDS tree that is NOT tied to a BB, and then use insert-medium to make the association after the fact? We don't need to create a BB named 'backend0' if we are immediately going to reuse 'node0' in a different BB (true, we have somewhat anticipated the idea of sharing BDS tree among multiple BB, but haven't quite turned that on before now). > + "driver": "raw", > + "file": { "driver": "file", > + "filename": "fedora.iso" } } } } > + > +<- { "return": {} } > + > +-> { "execute": "blockdev-insert-medium", > + "arguments": { "device": "ide1-cd0", > + "node-name": "node0" } } > + > +<- { "return": {} } If you can either explain why you used 'id' in the example, or remove that parameter, you can add: Reviewed-by: Eric Blake <eblake@redhat.com>
On 2015-01-28 at 15:18, Eric Blake wrote: > On 01/27/2015 12:46 PM, Max Reitz wrote: >> And a helper function for that which directly takes a pointer to the BDS >> to be inserted instead of its node-name (which will be used for >> implementing 'change' using blockdev-insert-medium). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> blockdev.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> qapi/block-core.json | 17 +++++++++++++++++ >> qmp-commands.hx | 38 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 98 insertions(+) >> >> +static void qmp_blockdev_insert_anon_medium(const char *device, >> + BlockDriverState *bs, Error **errp) >> +{ >> + BlockBackend *blk; >> + >> + blk = blk_by_name(device); >> + if (!blk) { >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> + } >> + >> + if (!blk_dev_has_removable_media(blk)) { >> + error_setg(errp, "Device '%s' is not removable", device); >> + return; >> + } >> + >> + if (!blk_dev_is_tray_open(blk)) { >> + error_setg(errp, "Tray of device '%s' is not open", device); >> + return; >> + } >> + >> + if (blk_bs(blk)) { >> + error_setg(errp, "There already is a medium in device '%s'", device); >> + return; >> + } > Good, you didn't implement hot-swap semantics of replacing an existing > medium (that gets too confusing; so I _like_ that you force the user to > consider all the steps through multiple low-level commands). > > >> + >> +Example (1): > I'll quit pointing it out; but if you clean up the useless (1) in one > patch, do it across the series. Yes, will do. >> + >> +-> { "execute": "blockdev-add", >> + "arguments": { "options": { "id": "backend0", >> + "node-name": "node0", > Why is 'id' needed? Oops, because I forgot removing it after I added the functionality for creating a BDS tree without a BB. I'll remove it, thanks for catching it. Max > Isn't the point of this command sequence to create > a BDS tree that is NOT tied to a BB, and then use insert-medium to make > the association after the fact? We don't need to create a BB named > 'backend0' if we are immediately going to reuse 'node0' in a different > BB (true, we have somewhat anticipated the idea of sharing BDS tree > among multiple BB, but haven't quite turned that on before now). > >> + "driver": "raw", >> + "file": { "driver": "file", >> + "filename": "fedora.iso" } } } } >> + >> +<- { "return": {} } >> + >> +-> { "execute": "blockdev-insert-medium", >> + "arguments": { "device": "ide1-cd0", >> + "node-name": "node0" } } >> + >> +<- { "return": {} } > If you can either explain why you used 'id' in the example, or remove > that parameter, you can add: > Reviewed-by: Eric Blake <eblake@redhat.com> >
diff --git a/blockdev.c b/blockdev.c index 17785b8..e4588b3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2088,6 +2088,49 @@ void qmp_blockdev_remove_medium(const char *device, Error **errp) } } +static void qmp_blockdev_insert_anon_medium(const char *device, + BlockDriverState *bs, Error **errp) +{ + BlockBackend *blk; + + blk = blk_by_name(device); + if (!blk) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; + } + + if (!blk_dev_has_removable_media(blk)) { + error_setg(errp, "Device '%s' is not removable", device); + return; + } + + if (!blk_dev_is_tray_open(blk)) { + error_setg(errp, "Tray of device '%s' is not open", device); + return; + } + + if (blk_bs(blk)) { + error_setg(errp, "There already is a medium in device '%s'", device); + return; + } + + blk_insert_bs(blk, bs); +} + +void qmp_blockdev_insert_medium(const char *device, const char *node_name, + Error **errp) +{ + BlockDriverState *bs; + + bs = bdrv_find_node(node_name); + if (!bs) { + error_setg(errp, "Node '%s' not found", node_name); + return; + } + + qmp_blockdev_insert_anon_medium(device, bs, errp); +} + /* throttling disk I/O limits */ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd, int64_t bps_wr, diff --git a/qapi/block-core.json b/qapi/block-core.json index 1ace69d..ba41015 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1766,6 +1766,23 @@ { 'command': 'blockdev-remove-medium', 'data': { 'device': 'str' } } +## +# @blockdev-insert-medium: +# +# Inserts a medium (a block driver state tree) into a block device. That block +# device's tray must currently be open and there must be no medium inserted +# already. +# +# @device: block device name +# +# @node-name: name of a node in the block driver state graph +# +# Since: 2.3 +## +{ 'command': 'blockdev-insert-medium', + 'data': { 'device': 'str', + 'node-name': 'str'} } + ## # @BlockErrorAction diff --git a/qmp-commands.hx b/qmp-commands.hx index 3d5c10c..604d638 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -3754,6 +3754,44 @@ Example (1): EQMP { + .name = "blockdev-insert-medium", + .args_type = "device:s,node-name:s", + .mhandler.cmd_new = qmp_marshal_input_blockdev_insert_medium, + }, + +SQMP +blockdev-insert-medium +---------------------- + +Inserts a medium (a block driver state tree) into a block device. That block +device's tray must currently be open and there must be no medium inserted +already. + +Arguments: + +- "device": block device name (json-string) +- "node-name": root node of the BDS tree to insert into the block device + +Example (1): + +-> { "execute": "blockdev-add", + "arguments": { "options": { "id": "backend0", + "node-name": "node0", + "driver": "raw", + "file": { "driver": "file", + "filename": "fedora.iso" } } } } + +<- { "return": {} } + +-> { "execute": "blockdev-insert-medium", + "arguments": { "device": "ide1-cd0", + "node-name": "node0" } } + +<- { "return": {} } + +EQMP + + { .name = "query-named-block-nodes", .args_type = "", .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
And a helper function for that which directly takes a pointer to the BDS to be inserted instead of its node-name (which will be used for implementing 'change' using blockdev-insert-medium). Signed-off-by: Max Reitz <mreitz@redhat.com> --- blockdev.c | 43 +++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 17 +++++++++++++++++ qmp-commands.hx | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+)