On 12/8/2019 8:58 PM, Max Reitz wrote:
> On 16.05.19 16:33, Anton Nefedov wrote:
>> it allows to report it in the error handler
>>
>> Signed-off-by: Anton Nefedov <anton.nefe...@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>> Reviewed-by: Alberto Garcia <be...@igalia.com>
>> ---
>> hw/scsi/scsi-disk.c | 12 +++++-------
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> (Sorry for the late reply :-/)
>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index e7e865ab3b..b43254103c 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData
>> *data, int ret)
>> {
>> SCSIDiskReq *r = data->r;
>> SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> - uint64_t sector_num;
>> - uint32_t nb_sectors;
>>
>> assert(r->req.aiocb == NULL);
>> if (scsi_disk_req_check_error(r, ret, false)) {
>> @@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData
>> *data, int ret)
>> }
>>
>> if (data->count > 0) {
>> - sector_num = ldq_be_p(&data->inbuf[0]);
>> - nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
>> - if (!check_lba_range(s, sector_num, nb_sectors)) {
>> + r->sector = ldq_be_p(&data->inbuf[0]);
>> + r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
>> + if (!check_lba_range(s, r->sector, r->sector_count)) {
>> scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
>> goto done;
>> }
>>
>> r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
>> - sector_num * s->qdev.blocksize,
>> - nb_sectors * s->qdev.blocksize,
>> + r->sector * s->qdev.blocksize,
>> + r->sector_count * s->qdev.blocksize,
>
> This looks to me like these are not necessarily in terms of 512-byte
> sectors. It doesn’t seem to make anything technically wrong, because
> patch 7 takes that into account.
>
> But it’s still weird if everything else in this file treats these fields
> as being in terms of 512 byte sectors (and they are actually defined
> this way in SCSIDiskReq).
>
Nice that you caught this, thanks! I guess variable names misled me
