Am 22.10.2015 um 18:17 schrieb Stefan Hajnoczi:
> On Mon, Oct 12, 2015 at 02:27:22PM +0200, Peter Lieven wrote:
>> @@ -129,9 +134,71 @@ static int cd_read_sector(IDEState *s, int lba, uint8_t
>> *buf, int sector_size)
>> ret = -EIO;
>> break;
>> }
>> +
>> + if (!ret) {
>> + s->lba++;
> This function probably shouldn't take the lba argument if it modifies
> s->lba. You dropped the sector_size argument and I think the lba
> argument should be dropped for the same reason.
>
>> +static int cd_read_sector(IDEState *s, int lba, void *buf)
>> +{
>> + if (s->cd_sector_size != 2048 && s->cd_sector_size != 2352) {
>> + return -EINVAL;
>> + }
>> +
>> + s->iov.iov_base = buf;
>> + if (s->cd_sector_size == 2352) {
>> + buf += 16;
>> + }
>> +
>> + s->iov.iov_len = 4 * BDRV_SECTOR_SIZE;
>> + qemu_iovec_init_external(&s->qiov, &s->iov, 1);
>> +
>> +#ifdef DEBUG_IDE_ATAPI
>> + printf("cd_read_sector: lba=%d\n", lba);
>> +#endif
>> +
>> + if (blk_aio_readv(s->blk, (int64_t)lba << 2, &s->qiov, 4,
>> + cd_read_sector_cb, s) == NULL) {
> This function never returns NULL, the if statement can be removed.
Okay, that was my fault. I was believing it could return NULL.
>
> Doesn't the aiocb need to be stored for cancellation (e.g. device reset)?
The ide_readv_cancelable function introduced in Patch 3 will store
the aiocb. At this point there is blk_drain_all called when the device is reset.
>
>> + return -EIO;
>> + }
>> +
>> + block_acct_start(blk_get_stats(s->blk), &s->acct,
>> + 4 * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
> Why does accounting start *after* the read request has been submitted?
You are right it has to start before the request.
Peter
