Am 13.10.2011 13:03, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> 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
