| Message ID | 1307127842-12102-6-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On (Fri) 03 Jun 2011 [16:03:57], Luiz Capitulino wrote: > +static int tray_open(const char *device, int remove, int force) > +{ > + BlockDriverState *bs; > + > + bs = bdrv_removable_find(device); > + if (!bs) { > + return -1; > + } > + > + if (bdrv_eject(bs, 1, force) < 0) { > + /* FIXME: will report undefined error in QMP */ > + return -1; > + } > + > + if (remove) { > + bdrv_close(bs); > + } > + > + return 0; > +} What's the reason to tie the 'remove' with tray open? Won't it be simpler to have it separated out, perhaps a 'change' event instead of 'insert' that can accept NULL which means just remove medium? Amit
Luiz Capitulino <lcapitulino@redhat.com> writes: > This command opens a removable media drive's tray. It's only available > in QMP. > > The do_tray_open() function is split into two smaller functions because > next commits will also use them. > > Please, check the command's documentation (being introduced in this > commit) for a detailed description. > > XXX: Should we return an error if the tray is already open? Use case? For what it's worth, Linux ioctl CDROMCLOSETRAY appears to return success when it does nothing.
On Mon, 6 Jun 2011 17:10:32 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Fri) 03 Jun 2011 [16:03:57], Luiz Capitulino wrote: > > > +static int tray_open(const char *device, int remove, int force) > > +{ > > + BlockDriverState *bs; > > + > > + bs = bdrv_removable_find(device); > > + if (!bs) { > > + return -1; > > + } > > + > > + if (bdrv_eject(bs, 1, force) < 0) { > > + /* FIXME: will report undefined error in QMP */ > > + return -1; > > + } > > + > > + if (remove) { > > + bdrv_close(bs); > > + } > > + > > + return 0; > > +} > > What's the reason to tie the 'remove' with tray open? In my first try I had a command called 'blockdev-media-remove', but then I had the impression that I was going too far as the only reason a client would ever want to open the tray is to remove the media. > Won't it be > simpler to have it separated out, perhaps a 'change' event instead of > 'insert' that can accept NULL which means just remove medium? You meant 'command' instead of 'event', right? I don't think a change command makes sense, because it's just a shortcut to open/remove/insert/close.
On (Mon) 06 Jun 2011 [11:38:03], Luiz Capitulino wrote: > On Mon, 6 Jun 2011 17:10:32 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > On (Fri) 03 Jun 2011 [16:03:57], Luiz Capitulino wrote: > > > > > +static int tray_open(const char *device, int remove, int force) > > > +{ > > > + BlockDriverState *bs; > > > + > > > + bs = bdrv_removable_find(device); > > > + if (!bs) { > > > + return -1; > > > + } > > > + > > > + if (bdrv_eject(bs, 1, force) < 0) { > > > + /* FIXME: will report undefined error in QMP */ > > > + return -1; > > > + } > > > + > > > + if (remove) { > > > + bdrv_close(bs); > > > + } > > > + > > > + return 0; > > > +} > > > > What's the reason to tie the 'remove' with tray open? > > In my first try I had a command called 'blockdev-media-remove', but then > I had the impression that I was going too far as the only reason a client > would ever want to open the tray is to remove the media. Not necessary -- CD/DVD writers eject and reload trays after erasing media -- at least they used to. > > Won't it be > > simpler to have it separated out, perhaps a 'change' event instead of > > 'insert' that can accept NULL which means just remove medium? > > You meant 'command' instead of 'event', right? > > I don't think a change command makes sense, because it's just a shortcut > to open/remove/insert/close. Yes, command, sorry. And by 'change' I don't mean the current monitor change command -- that's a badly-named one. By change I mean just that -- replace the media. And that should succeed only if tray is open. And tray remains open after the change. Amit
On Tue, 7 Jun 2011 18:59:12 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Mon) 06 Jun 2011 [11:38:03], Luiz Capitulino wrote: > > On Mon, 6 Jun 2011 17:10:32 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > On (Fri) 03 Jun 2011 [16:03:57], Luiz Capitulino wrote: > > > > > > > +static int tray_open(const char *device, int remove, int force) > > > > +{ > > > > + BlockDriverState *bs; > > > > + > > > > + bs = bdrv_removable_find(device); > > > > + if (!bs) { > > > > + return -1; > > > > + } > > > > + > > > > + if (bdrv_eject(bs, 1, force) < 0) { > > > > + /* FIXME: will report undefined error in QMP */ > > > > + return -1; > > > > + } > > > > + > > > > + if (remove) { > > > > + bdrv_close(bs); > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > > > What's the reason to tie the 'remove' with tray open? > > > > In my first try I had a command called 'blockdev-media-remove', but then > > I had the impression that I was going too far as the only reason a client > > would ever want to open the tray is to remove the media. > > Not necessary -- CD/DVD writers eject and reload trays after erasing > media -- at least they used to. But that's not done by the VM's user/client. I think I'll add it anyway, as it's what people seem to expect from an API like this. > > > Won't it be > > > simpler to have it separated out, perhaps a 'change' event instead of > > > 'insert' that can accept NULL which means just remove medium? > > > > You meant 'command' instead of 'event', right? > > > > I don't think a change command makes sense, because it's just a shortcut > > to open/remove/insert/close. > > Yes, command, sorry. > > And by 'change' I don't mean the current monitor change command -- > that's a badly-named one. > > By change I mean just that -- replace the media. And that should > succeed only if tray is open. And tray remains open after the change. The same thing can be done with blockdev-media-remove and blockdev-media-insert, so I don't think this adds value.
diff --git a/blockdev.c b/blockdev.c index 6e0eb83..b1c705c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -665,6 +665,52 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force) return 0; } +static BlockDriverState *bdrv_removable_find(const char *name) +{ + BlockDriverState *bs; + + bs = bdrv_find(name); + if (!bs) { + qerror_report(QERR_DEVICE_NOT_FOUND, name); + return NULL; + } + + if (!bdrv_is_removable(bs)) { + qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); + return NULL; + } + + return bs; +} + +static int tray_open(const char *device, int remove, int force) +{ + BlockDriverState *bs; + + bs = bdrv_removable_find(device); + if (!bs) { + return -1; + } + + if (bdrv_eject(bs, 1, force) < 0) { + /* FIXME: will report undefined error in QMP */ + return -1; + } + + if (remove) { + bdrv_close(bs); + } + + return 0; +} + +int do_tray_open(Monitor *mon, const QDict *qdict, QObject **ret_data) +{ + return tray_open(qdict_get_str(qdict, "device"), + qdict_get_try_bool(qdict, "remove", 0), + qdict_get_try_bool(qdict, "force", 0)); +} + int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data) { BlockDriverState *bs; diff --git a/blockdev.h b/blockdev.h index 3587786..5e46aae 100644 --- a/blockdev.h +++ b/blockdev.h @@ -65,5 +65,6 @@ int do_change_block(Monitor *mon, const char *device, int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data); int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data); +int do_tray_open(Monitor *mon, const QDict *qdict, QObject **ret_data); #endif diff --git a/qmp-commands.hx b/qmp-commands.hx index 8393f22..58ab132 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -153,6 +153,33 @@ Examples: EQMP { + .name = "blockdev-tray-open", + .args_type = "device:B,force:-f,remove:-r", + .mhandler.cmd_new = do_tray_open, + }, + +SQMP +blockdev-tray-open +------------------ + +Open a removable media drive's tray. + +Arguments: + +- device: device name (json-string) +- force: force ejection (json-bool, optional) +- remove: remove the media (json-bool, optional) + +Example: + +-> { "execute": "blockdev-tray-open", "arguments": { "device": "ide1-cd0" } } +<- { "return": {} } + +Note: The tray remains open after this command is issued + +EQMP + + { .name = "screendump", .args_type = "filename:F", .params = "filename",
This command opens a removable media drive's tray. It's only available in QMP. The do_tray_open() function is split into two smaller functions because next commits will also use them. Please, check the command's documentation (being introduced in this commit) for a detailed description. XXX: Should we return an error if the tray is already open? Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- blockdev.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ blockdev.h | 1 + qmp-commands.hx | 27 +++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 0 deletions(-)