From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1LSBCh-0006Nt-Md
	for qemu-devel@nongnu.org; Wed, 28 Jan 2009 09:15:43 -0500
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1LSBCg-0006LC-6V
	for qemu-devel@nongnu.org; Wed, 28 Jan 2009 09:15:43 -0500
Received: from [199.232.76.173] (port=47803 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1LSBCg-0006L9-0U
	for qemu-devel@nongnu.org; Wed, 28 Jan 2009 09:15:42 -0500
Received: from mx2.redhat.com ([66.187.237.31]:38571)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <riel@redhat.com>) id 1LSBCf-0004QT-IL
	for qemu-devel@nongnu.org; Wed, 28 Jan 2009 09:15:41 -0500
Message-ID: <49806811.4070407@redhat.com>
Date: Wed, 28 Jan 2009 09:13:37 -0500
From: Rik van Riel <riel@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH] support >2TB SCSI disks
References: <20090127224619.3ae16173@bree.surriel.com>
	<200901281230.29455.paul@codesourcery.com>
In-Reply-To: <200901281230.29455.paul@codesourcery.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Reply-To: qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paul Brook <paul@codesourcery.com>
Cc: qemu-devel@nongnu.org

Paul Brook wrote:
>>      case 0:
>> -        lba = buf[3] | (buf[2] << 8) | ((buf[1] & 0x1f) << 16);
>> +        lba = (uint64_t) buf[3] | ((uint64_t) buf[2] << 8) |
>> +              (((uint64_t) buf[1] & 0x1f) << 16);
> 
> This is not required, though I guess it's harmless.

I thought I'd keep them all consistent :)

>>      case 4:
>> ...
>>          len = buf[13] | (buf[12] << 8) | (buf[11] << 16) | (buf[10] << 24);
>> ...
>> +    case 0x88:
>>          r->sector_count = len * s->cluster_size;
> 
> Implementing these commands introduces several overflows. There are several 
> places (including SCSIRequest->sector_count and the return value from 
> scsi_send_comand) that assume the transfer length fits in a signed (32-bit) 
> int.

True, a SCSI transfer of more than 2GB would cause an overflow.

>> +            /* Returned value is the address of the last sector.  */
>> +            if (nb_sectors) {
>> +                nb_sectors--;
> 
> By my reading both this and the current Read Capacity(10)  are incorrect.
> They need to divide by s->cluster_size.

Good point.  Want me to send in a separate patch that does that?

-- 
All rights reversed.