Re: [Qemu-devel] [PATCH 11/35] scsi-disk: support READ DVD STRUCTURE

[Kevin Wolf]

Am 13.10.2011 13:03, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/scsi-disk.c |  101 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 100 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 1786c37..14db6a0 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -576,10 +576,109 @@ static inline bool media_is_dvd(SCSIDiskState *s)
>      return nb_sectors > CD_MAX_SECTORS;
>  }
>  
> +static inline bool media_is_cd(SCSIDiskState *s)
> +{
> +    uint64_t nb_sectors;
> +    if (s->qdev.type != TYPE_ROM) {
> +        return false;
> +    }
> +    if (!bdrv_is_inserted(s->bs)) {
> +        return false;
> +    }
> +    bdrv_get_geometry(s->bs, &nb_sectors);
> +    return nb_sectors <= CD_MAX_SECTORS;
> +}
> +
>  static int scsi_read_dvd_structure(SCSIDiskState *s, SCSIDiskReq *r,
>                                     uint8_t *outbuf)
>  {
> -    scsi_check_condition(r, SENSE_CODE(INVALID_OPCODE));
> +    static const int rds_caps_size[5] = {
> +        [0] = 2048 + 4,
> +        [1] = 4 + 4,
> +        [3] = 188 + 4,
> +        [4] = 2048 + 4,
> +    };
> +
> +    uint8_t media = r->req.cmd.buf[1];
> +    uint8_t layer = r->req.cmd.buf[6];
> +    uint8_t format = r->req.cmd.buf[7];
> +    int size = -1;
> +
> +    if (s->qdev.type != TYPE_ROM || !bdrv_is_inserted(s->bs)) {
> +        return -1;
> +    }
> +    if (s->tray_open || !bdrv_is_inserted(s->bs)) {
> +        scsi_check_condition(r, SENSE_CODE(NO_MEDIUM));
> +        return -1;
> +    }

You are checking twice for bdrv_is_inserted, which one do you really mean?

Also, format = 0xff should work even without a medium.

> +    if (media_is_cd(s)) {
> +        scsi_check_condition(r, SENSE_CODE(INCOMPATIBLE_FORMAT));
> +        return -1;
> +    }
> +    if (media != 0) {
> +        scsi_check_condition(r, SENSE_CODE(INCOMPATIBLE_FORMAT));
> +        return -1;
> +    }
> +
> +    if (format != 0xff) {
> +        if (format >= sizeof(rds_caps_size) / sizeof(rds_caps_size[0])) {

osdep.h has an ARRAY_SIZE() macro.

> +            return -1;
> +        }
> +        size = rds_caps_size[format];
> +        memset(outbuf, 0, size);
> +    }
> +
> +    switch (format) {
> +    case 0x00: {
> +        /* Physical format information */
> +        uint64_t nb_sectors;
> +        if (layer != 0)
> +            goto fail;

Braces

> +        bdrv_get_geometry(s->bs, &nb_sectors);
> +
> +        outbuf[4] = 1;   /* DVD-ROM, part version 1 */
> +        outbuf[5] = 0xf; /* 120mm disc, minimum rate unspecified */
> +        outbuf[6] = 1;   /* one layer, read-only (per MMC-2 spec) */
> +        outbuf[7] = 0;   /* default densities */
> +
> +        stl_be_p(&outbuf[12], (nb_sectors >> 2) - 1); /* end sector */
> +        stl_be_p(&outbuf[16], (nb_sectors >> 2) - 1); /* l0 end sector */
> +        break;
> +    }
> +
> +    case 0x01: /* DVD copyright information, all zeros */
> +        break;
> +
> +    case 0x03: /* BCA information - invalid field for no BCA info */
> +        return -1;
> +
> +    case 0x04: /* DVD disc manufacturing information, all zeros */
> +        break;
> +
> +    case 0xff: { /* List capabilities */
> +        int i;
> +        size = 4;
> +        for (i = 0; i < sizeof(rds_caps_size) / sizeof(rds_caps_size[0]); 
> i++) {

ARRAY_SIZE() again

> +            if (!rds_caps_size[i]) {
> +                continue;
> +            }
> +            outbuf[size] = i;
> +            outbuf[size + 1] = 0x40; /* Not writable, readable */
> +            stw_be_p(&outbuf[size + 2], rds_caps_size[i]);
> +            size += 4;
> +        }
> +        break;
> +     }
> +
> +    default:
> +        return -1;
> +    }
> +
> +    /* Size of buffer, not including 2 byte size field */
> +    stw_be_p(outbuf, size - 2);
> +    return size;
> +
> +fail:
>      return -1;
>  }

There is only one 'goto fail', all other places have a direct return -1.
It would be good to be consistent.

Also, as this is mostly a refactored copy from the ATAPI code, I wonder
what our long-term plan is. At which point will we be able to unify what
we're duplicating right now? Can we share some parts even now?

Kevin