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 <mre...@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
pgpsEdtIPlKJW.pgp
Description: PGP signature