Re: [Qemu-devel] [PATCH v3 16/44] atapi: Switch to byte-based block access

[John Snow]

On 04/22/2016 07:40 PM, Eric Blake wrote:
> Sector-based blk_read() should die; switch to byte-based
> blk_pread() instead.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  hw/ide/atapi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 2bb606c..81000d8 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -119,12 +119,12 @@ cd_read_sector_sync(IDEState *s)
> 
>      switch (s->cd_sector_size) {
>      case 2048:
> -        ret = blk_read(s->blk, (int64_t)s->lba << 2,
> -                       s->io_buffer, 4);
> +        ret = blk_pread(s->blk, (int64_t)s->lba << (2 + BDRV_SECTOR_BITS),
> +                        s->io_buffer, 4 << BDRV_SECTOR_BITS);
>          break;
>      case 2352:
> -        ret = blk_read(s->blk, (int64_t)s->lba << 2,
> -                       s->io_buffer + 16, 4);
> +        ret = blk_pread(s->blk, (int64_t)s->lba << (2 + BDRV_SECTOR_BITS),

Uh, hm. So what we have is a cdrom-sector-based LBA, that we need to
transform into IDE-based sectors, then to bytes.

We could just define an ATAPI_SECTOR_BITS to be (2 + BDRV_SECTOR_BITS).

Then, the lba conversion would be just:

s->lba << ATAPI_SECTOR_BITS

and the size would be just:

1 << ATAPI_SECTOR_BITS

And that's probably better on the eyes.

> +                        s->io_buffer + 16, 4 << BDRV_SECTOR_BITS);
>          if (ret >= 0) {
>              cd_data_to_raw(s->io_buffer, s->lba);
>          }
> 

The code already uses lots of different stuff like
4 * BDRV_SECTOR_SIZE
or
"2048"

so we probably need some staple definition here.


...Otherwise, none of this is a problem you've created, just one this
patch highlights. Fix at your own peril.

Acked-by: John Snow <address@hidden>