| Message ID | 1445270025-22999-32-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon 19 Oct 2015 05:53:37 PM CEST, 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). Shouldn't this update bdrv_states? Consider this scenario: 1) We add a drive and eject its BDS { "execute": "blockdev-add", "arguments": { "options": { "driver": "qcow2", "file": { "driver": "file", "filename": "/tmp/hd0.img"}, "id": "drive0" }}} { "execute": "eject", "arguments": {"device": "drive0"}} 2) We create a new BDS and insert it in drive0 { "execute": "blockdev-add", "arguments": { "options": { "driver": "qcow2", "file": { "driver": "file", "filename": "/tmp/hd0.img"}, "node-name": "hd0" }}} { "execute": "blockdev-insert-medium", "arguments": { "device": "drive0", "node-name": "hd0" }} 3) Now we try to create a snapshot... { "execute": "blockdev-snapshot-sync", "arguments": { "device": "drive0", "snapshot-file": "/tmp/new.img", "format": "qcow2" }} {"error": {"class": "GenericError", "desc": "The feature 'snapshot' is not enabled"}} Ooops! It seems that this is because the new node hd0 is not in the bdrv_states list. 4) Let's try to create a mirror instead { "execute": "drive-mirror", "arguments": { "device": "drive0", "target": "/tmp/new.img", "sync": "top"}} {"return": {}} {"timestamp": {"seconds": 1445427560, "microseconds": 765993}, "event": "BLOCK_JOB_READY", "data": {"device": "drive0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} 5) Ok, the block job is ready, so let's complete it: { "execute": "query-block-jobs" } {"return": []} Ooops! Again, hd0 is not in bdrv_states so QEMU cannot find the block job. 6) Anyway, we only need the backend name in order to complete a block job, so surely we can do it even if it's not in the list: { "execute": "block-job-complete", "arguments": { "device": "drive0"}} Segmentation fault That's QTAILQ_INSERT_BEFORE() in change_parent_backing_link(). This code assumes that since the 'from' BDS is attached to a backend, it must also be in bdrv_states. Berto
On 21.10.2015 13:49, Alberto Garcia wrote: > On Mon 19 Oct 2015 05:53:37 PM CEST, 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). > > Shouldn't this update bdrv_states? I hate bdrv_states. Yes, it should. Thanks! Max > Consider this scenario: > > 1) We add a drive and eject its BDS > { "execute": "blockdev-add", "arguments": { > "options": { "driver": "qcow2", > "file": { "driver": "file", > "filename": "/tmp/hd0.img"}, > "id": "drive0" }}} > > { "execute": "eject", "arguments": {"device": "drive0"}} > > 2) We create a new BDS and insert it in drive0 > { "execute": "blockdev-add", "arguments": { > "options": { "driver": "qcow2", > "file": { "driver": "file", > "filename": "/tmp/hd0.img"}, > "node-name": "hd0" }}} > > { "execute": "blockdev-insert-medium", "arguments": { > "device": "drive0", > "node-name": "hd0" }} > > 3) Now we try to create a snapshot... > > { "execute": "blockdev-snapshot-sync", "arguments": { > "device": "drive0", > "snapshot-file": "/tmp/new.img", > "format": "qcow2" }} > > {"error": {"class": "GenericError", > "desc": "The feature 'snapshot' is not enabled"}} > > Ooops! It seems that this is because the new node hd0 is not in the > bdrv_states list. > > 4) Let's try to create a mirror instead > > { "execute": "drive-mirror", "arguments": { > "device": "drive0", > "target": "/tmp/new.img", > "sync": "top"}} > > {"return": {}} > {"timestamp": {"seconds": 1445427560, > "microseconds": 765993}, > "event": "BLOCK_JOB_READY", > "data": {"device": "drive0", > "len": 0, > "offset": 0, > "speed": 0, > "type": "mirror"}} > > 5) Ok, the block job is ready, so let's complete it: > > { "execute": "query-block-jobs" } > {"return": []} > > Ooops! Again, hd0 is not in bdrv_states so QEMU cannot find the block > job. > > 6) Anyway, we only need the backend name in order to complete a block > job, so surely we can do it even if it's not in the list: > > { "execute": "block-job-complete", "arguments": { > "device": "drive0"}} > > Segmentation fault > > That's QTAILQ_INSERT_BEFORE() in change_parent_backing_link(). This > code assumes that since the 'from' BDS is attached to a backend, it > must also be in bdrv_states. > > Berto >
Am 21.10.2015 um 15:47 hat Max Reitz geschrieben: > On 21.10.2015 13:49, Alberto Garcia wrote: > > On Mon 19 Oct 2015 05:53:37 PM CEST, 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). > > > > Shouldn't this update bdrv_states? > > I hate bdrv_states. > > Yes, it should. Thanks! Once your reimplement blk_set_bs() on top of blk_insert/remove_bs(), this logic would replace the code in change_parent_backing_link(). Of course, I left the list update in block.c for a reason, it's meant to be an internal data structure, so your code accessing it from outside won't be much nicer. Do we actually still need bdrv_states or can we get rid of it in a follow-up series? If so, I wouldn't mind an ugly intermediate state. Kevin
Am 19.10.2015 um 17:53 hat Max Reitz geschrieben: > 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 | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 17 +++++++++++++++++ > qmp-commands.hx | 37 +++++++++++++++++++++++++++++++++++ > 3 files changed, 108 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index a8601ca..a4c278f 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2151,6 +2151,60 @@ 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; > + bool has_device; > + > + blk = blk_by_name(device); > + if (!blk) { > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Device '%s' not found", device); > + return; > + } > + > + /* For BBs without a device, we can exchange the BDS tree at will */ > + has_device = blk_get_attached_dev(blk); > + > + if (has_device && !blk_dev_has_removable_media(blk)) { > + error_setg(errp, "Device '%s' is not removable", device); > + return; > + } > + > + if (has_device && !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); Shouldn't this use bdrv_lookup_bs() to accept a BB name and be consisent with most other commands? Of course, if you actually used a BB name, it would fail below, but not with a confusing "not found" message. > + if (!bs) { > + error_setg(errp, "Node '%s' not found", node_name); > + return; > + } > + > + if (bs->blk) { > + error_setg(errp, "Node '%s' is already in use by '%s'", node_name, > + blk_name(bs->blk)); > + return; > + } > + > + qmp_blockdev_insert_anon_medium(device, bs, errp); > +} Kevin
On 23.10.2015 15:39, Kevin Wolf wrote: > Am 21.10.2015 um 15:47 hat Max Reitz geschrieben: >> On 21.10.2015 13:49, Alberto Garcia wrote: >>> On Mon 19 Oct 2015 05:53:37 PM CEST, 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). >>> >>> Shouldn't this update bdrv_states? >> >> I hate bdrv_states. >> >> Yes, it should. Thanks! > > Once your reimplement blk_set_bs() on top of blk_insert/remove_bs(), > this logic would replace the code in change_parent_backing_link(). > > Of course, I left the list update in block.c for a reason, it's meant to > be an internal data structure, so your code accessing it from outside > won't be much nicer. Do we actually still need bdrv_states or can we get > rid of it in a follow-up series? If so, I wouldn't mind an ugly > intermediate state. I do get rid of it in "blockdev: Further BlockBackend work"* (the final patch of that series). Max * http://lists.nongnu.org/archive/html/qemu-block/2015-02/msg00021.html
On 23.10.2015 15:42, Kevin Wolf wrote: > Am 19.10.2015 um 17:53 hat Max Reitz geschrieben: >> 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 | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qapi/block-core.json | 17 +++++++++++++++++ >> qmp-commands.hx | 37 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 108 insertions(+) >> >> diff --git a/blockdev.c b/blockdev.c >> index a8601ca..a4c278f 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2151,6 +2151,60 @@ 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; >> + bool has_device; >> + >> + blk = blk_by_name(device); >> + if (!blk) { >> + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >> + "Device '%s' not found", device); >> + return; >> + } >> + >> + /* For BBs without a device, we can exchange the BDS tree at will */ >> + has_device = blk_get_attached_dev(blk); >> + >> + if (has_device && !blk_dev_has_removable_media(blk)) { >> + error_setg(errp, "Device '%s' is not removable", device); >> + return; >> + } >> + >> + if (has_device && !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); > > Shouldn't this use bdrv_lookup_bs() to accept a BB name and be consisent > with most other commands? Of course, if you actually used a BB name, it > would fail below, but not with a confusing "not found" message. Well, and then it fails with "Node 'foo' is already in use by 'foo'", which doesn't make much more sense either. Until we support multiple BBs per BDS, using this command with a BB doesn't make any sense. I may be wrong here or exaggerating, but I feel like most of the "most other commands" allow that mostly because of legacy reasons. Second, most of them are block jobs which I feel like should work behind a BB anyway, and letting them work on a BDS is wrong, but we just haven't converted them yet. I don't have a strong preference. I find the error messages equally bad. But I think I don't want to use bdrv_lookup_bs() since that would look like pretending that we actually do want to support BB names, whereas in reality we absolutely don't (not right now at least). Also, it would confuse me when reading the code: "Why are you accepting a BB name up there, and then you are rejecting every BDS that has a BB? That doesn't make sense!" Improving the error message doesn't seem to good to me either. It would have to be something like "'%s' is a device, not a node" which I don't consider much more helpful either (maybe it is, I don't know), and adding an explanation like "; blockdev-insert-medium only accepts node names" would feel like a bit too much since we don't do that anywhere else, do we? Max >> + if (!bs) { >> + error_setg(errp, "Node '%s' not found", node_name); >> + return; >> + } >> + >> + if (bs->blk) { >> + error_setg(errp, "Node '%s' is already in use by '%s'", node_name, >> + blk_name(bs->blk)); >> + return; >> + } >> + >> + qmp_blockdev_insert_anon_medium(device, bs, errp); >> +} > > Kevin >
Am 23.10.2015 um 16:35 hat Max Reitz geschrieben: > On 23.10.2015 15:42, Kevin Wolf wrote: > > Am 19.10.2015 um 17:53 hat Max Reitz geschrieben: > >> 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 | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> qapi/block-core.json | 17 +++++++++++++++++ > >> qmp-commands.hx | 37 +++++++++++++++++++++++++++++++++++ > >> 3 files changed, 108 insertions(+) > >> > >> diff --git a/blockdev.c b/blockdev.c > >> index a8601ca..a4c278f 100644 > >> --- a/blockdev.c > >> +++ b/blockdev.c > >> @@ -2151,6 +2151,60 @@ 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; > >> + bool has_device; > >> + > >> + blk = blk_by_name(device); > >> + if (!blk) { > >> + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > >> + "Device '%s' not found", device); > >> + return; > >> + } > >> + > >> + /* For BBs without a device, we can exchange the BDS tree at will */ > >> + has_device = blk_get_attached_dev(blk); > >> + > >> + if (has_device && !blk_dev_has_removable_media(blk)) { > >> + error_setg(errp, "Device '%s' is not removable", device); > >> + return; > >> + } > >> + > >> + if (has_device && !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); > > > > Shouldn't this use bdrv_lookup_bs() to accept a BB name and be consisent > > with most other commands? Of course, if you actually used a BB name, it > > would fail below, but not with a confusing "not found" message. > > Well, and then it fails with "Node 'foo' is already in use by 'foo'", > which doesn't make much more sense either. > > Until we support multiple BBs per BDS, using this command with a BB > doesn't make any sense. Correct, this would be mostly in preparation for supporting multiple BBs per BDS. > I may be wrong here or exaggerating, but I feel > like most of the "most other commands" allow that mostly because of > legacy reasons. Second, most of them are block jobs which I feel like > should work behind a BB anyway, and letting them work on a BDS is wrong, > but we just haven't converted them yet. > > I don't have a strong preference. I find the error messages equally bad. > But I think I don't want to use bdrv_lookup_bs() since that would look > like pretending that we actually do want to support BB names, whereas in > reality we absolutely don't (not right now at least). > > Also, it would confuse me when reading the code: "Why are you accepting > a BB name up there, and then you are rejecting every BDS that has a BB? > That doesn't make sense!" > > Improving the error message doesn't seem to good to me either. It would > have to be something like "'%s' is a device, not a node" which I don't > consider much more helpful either (maybe it is, I don't know), and > adding an explanation like "; blockdev-insert-medium only accepts node > names" would feel like a bit too much since we don't do that anywhere > else, do we? Fair enough. It's your code, you decide. Kevin
diff --git a/blockdev.c b/blockdev.c index a8601ca..a4c278f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2151,6 +2151,60 @@ 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; + bool has_device; + + blk = blk_by_name(device); + if (!blk) { + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", device); + return; + } + + /* For BBs without a device, we can exchange the BDS tree at will */ + has_device = blk_get_attached_dev(blk); + + if (has_device && !blk_dev_has_removable_media(blk)) { + error_setg(errp, "Device '%s' is not removable", device); + return; + } + + if (has_device && !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; + } + + if (bs->blk) { + error_setg(errp, "Node '%s' is already in use by '%s'", node_name, + blk_name(bs->blk)); + 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 8edf5d9..81a1f19 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1930,6 +1930,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.5 +## +{ 'command': 'blockdev-insert-medium', + 'data': { 'device': 'str', + 'node-name': 'str'} } + ## # @BlockErrorAction diff --git a/qmp-commands.hx b/qmp-commands.hx index ca46c80..16d7e2a 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -4055,6 +4055,43 @@ Example: EQMP { + .name = "blockdev-insert-medium", + .args_type = "device:s,node-name:s", + .mhandler.cmd_new = qmp_marshal_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: + +-> { "execute": "blockdev-add", + "arguments": { "options": { "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_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 | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 17 +++++++++++++++++ qmp-commands.hx | 37 +++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+)