On Thu, 21 Jul 2011 17:16:13 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes:
>
> > On Wed, 20 Jul 2011 18:24:02 +0200
> > Markus Armbruster <arm...@redhat.com> wrote:
> >
> >> The device model knows best when to accept the guest's eject command.
> >> No need to detour through the block layer.
> >>
> >> bdrv_eject() can't fail anymore. Make it void.
> >
> > But we're supposed to return an error to the user/client if we're unable
> > to eject, aren't we?
>
> Same story as for bdrv_set_locked() [PATCH 07/55]: the purpose is to
> synchronize the physical tray with the virtual one. Errors would have
> to be propagated up into device model init, post migration and guest
> START STOP UNIT. What error to report to the guest? Hardware failure?
>
> As with bdrv_set_locked(), I'm not removing any error handling. I don't
> add any either. The series is long enough...
I see.
>
> > (one more question below)
>
> Where?
This was to check if you'd find it where it's hidden.
Kidding :) The question I had was answered by a later patch, but I forgot
to remove the comment.
>
> >> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> >> ---
> >> block.c | 7 +------
> >> block.h | 2 +-
> >> hw/ide/atapi.c | 29 +++++++++--------------------
> >> hw/scsi-disk.c | 3 +++
> >> 4 files changed, 14 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index 6759066..70928af 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -2778,18 +2778,13 @@ int bdrv_media_changed(BlockDriverState *bs)
> >> /**
> >> * If eject_flag is TRUE, eject the media. Otherwise, close the tray
> >> */
> >> -int bdrv_eject(BlockDriverState *bs, int eject_flag)
> >> +void bdrv_eject(BlockDriverState *bs, int eject_flag)
> >> {
> >> BlockDriver *drv = bs->drv;
> >>
> >> - if (eject_flag && bs->locked) {
> >> - return -EBUSY;
> >> - }
> >> -
> >> if (drv && drv->bdrv_eject) {
> >> drv->bdrv_eject(bs, eject_flag);
> >> }
> >> - return 0;
> >> }
> >>
> >> int bdrv_is_locked(BlockDriverState *bs)
> >> diff --git a/block.h b/block.h
> >> index 65a0115..7cc7919 100644
> >> --- a/block.h
> >> +++ b/block.h
> >> @@ -209,7 +209,7 @@ int bdrv_is_inserted(BlockDriverState *bs);
> >> int bdrv_media_changed(BlockDriverState *bs);
> >> int bdrv_is_locked(BlockDriverState *bs);
> >> void bdrv_set_locked(BlockDriverState *bs, int locked);
> >> -int bdrv_eject(BlockDriverState *bs, int eject_flag);
> >> +void bdrv_eject(BlockDriverState *bs, int eject_flag);
> >> void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size);
> >> BlockDriverState *bdrv_find(const char *name);
> >> BlockDriverState *bdrv_next(BlockDriverState *bs);
> >> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> >> index 237657f..6cb8f0e 100644
> >> --- a/hw/ide/atapi.c
> >> +++ b/hw/ide/atapi.c
> >> @@ -894,33 +894,22 @@ static void cmd_seek(IDEState *s, uint8_t* buf)
> >>
> >> static void cmd_start_stop_unit(IDEState *s, uint8_t* buf)
> >> {
> >> - int sense, err = 0;
> >> + int sense;
> >> bool start = buf[4] & 1;
> >> bool loej = buf[4] & 2;
> >>
> >> if (loej) {
> >> - err = bdrv_eject(s->bs, !start);
> >> - }
> >> -
> >> - switch (err) {
> >> - case 0:
> >> - ide_atapi_cmd_ok(s);
> >> - break;
> >> - case -EBUSY:
> >> - sense = SENSE_NOT_READY;
> >> - if (bdrv_is_inserted(s->bs)) {
> >> - sense = SENSE_ILLEGAL_REQUEST;
> >> + if (!start && s->tray_locked) {
> >> + sense = bdrv_is_inserted(s->bs)
> >> + ? SENSE_NOT_READY : SENSE_ILLEGAL_REQUEST;
> >> + ide_atapi_cmd_error(s, sense, ASC_MEDIA_REMOVAL_PREVENTED);
> >> + return;
> >> }
> >> - ide_atapi_cmd_error(s, sense, ASC_MEDIA_REMOVAL_PREVENTED);
> >> - break;
> >> - default:
> >> - ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> >> - break;
> >> - }
> >> -
> >> - if (loej && !err) {
> >> + bdrv_eject(s->bs, !start);
> >> s->tray_open = !start;
> >> }
> >> +
> >> + ide_atapi_cmd_ok(s);
> >> }
> >>
> >> static void cmd_mechanism_status(IDEState *s, uint8_t* buf)
> >> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> >> index aac63b6..a4ed499 100644
> >> --- a/hw/scsi-disk.c
> >> +++ b/hw/scsi-disk.c
> >> @@ -833,6 +833,9 @@ static void scsi_disk_emulate_start_stop(SCSIDiskReq
> >> *r)
> >> bool loej = req->cmd.buf[4] & 2;
> >>
> >> if (s->drive_kind == SCSI_CD && loej) {
> >> + if (!start && s->tray_locked) {
> >> + return;
> >> + }
> >> bdrv_eject(s->bs, !start);
> >> s->tray_open = !start;
> >> }
>
