Re: [Qemu-devel] [PATCH 28/55] block: Leave enforcing tray lock to device models

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 21 Jul 2011 17:16:13 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 20 Jul 2011 18:24:02 +0200
> > Markus Armbruster <armbru@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 <armbru@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;
> >>      }
>