[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors
From: |
Anton Nefedov |
Subject: |
Re: [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct |
Date: |
Wed, 21 Aug 2019 11:03:11 +0000 |
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