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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Wed, 20 Jul 2011 18:24:02 +0200
> Markus Armbruster <address@hidden> 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...

> (one more question below)

Where?

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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;
>>      }