Re: [Qemu-block] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct

[Anton Nefedov]

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 <address@hidden>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> Reviewed-by: Alberto Garcia <address@hidden>
>> ---
>>   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