Re: [PATCH 6/6] scsi_scan: Fixup scsilun_to_int()

[Douglas Gilbert <dgilbert@interlog.com>]

On 14-06-10 07:37 AM, Bart Van Assche wrote:
> On 06/03/14 10:58, Hannes Reinecke wrote:
>> + *     Given a struct scsi_lun of: d2 04 0b 03 00 00 00 00, this function
>> + *     returns the integer: 0x0b03d204
>> + *
>> + *     This encoding will return a standard integer LUN for LUNs smaller
>> + *     than 256, which typically use a single level LUN structure with
>> + *     addressing method 0.
>>    **/
>>   u64 scsilun_to_int(struct scsi_lun *scsilun)
>>   {
>> @@ -1279,7 +1280,7 @@ u64 scsilun_to_int(struct scsi_lun *scsilun)
>>
>>   	lun = 0;
>>   	for (i = 0; i < sizeof(lun); i += 2)
>> -		lun = lun | (((scsilun->scsi_lun[i] << 8) |
>> +		lun = lun | (((scsilun->scsi_lun[i] << ((i + 1) *8)) |
>>   			      scsilun->scsi_lun[i + 1]) << (i * 8));
>>   	return lun;
>>   }
>
> The above code doesn't match the comment header. Parentheses have been
> placed such that each byte with an even index is shifted left (2*i+1)*8
> bits instead of (i+1)*8. I assume this means the parentheses have been
> misplaced ? Another bug in this code is that a cast from
> scsilun->scsi_lun[i] from u8 to u64 is missing. I think the shift
> operations in the above code trigger what is called undefined behavior
> in the C standard if i >= 4.

Hmm, we have been over this ground before. In sg_luns,
to support translating between the T10 and Linux
representation of 64 bit LUNs, there are these two
versions:


static uint64_t
t10_2linux_lun(const unsigned char t10_lun[])
{
     const unsigned char * cp;
     uint64_t res;

      res = (t10_lun[6] << 8) + t10_lun[7];
      for (cp = t10_lun + 4; cp >= t10_lun; cp -= 2) {
         res <<= 16;
         res += (*cp << 8) + *(cp + 1);
     }
     return res;
}

/* Copy of t10_lun --> Linux unsigned int (i.e. 32 bit ) present in Linux
  * kernel, up to least lk 3.8.0, extended to 64 bits.
  * BEWARE: for sizeof(int==4) this function is BROKEN and is left here as
  * as example and may soon be removed. */
static uint64_t
t10_2linux_lun64bitBR(const unsigned char t10_lun[])
{
     int i;
     uint64_t lun;

     lun = 0;
     for (i = 0; i < (int)sizeof(lun); i += 2)
         lun = lun | (((t10_lun[i] << 8) | t10_lun[i + 1]) << (i * 8));
     return lun;
}

The second one (with "BR" (for broken) appended) is for testing.
And that second one looks very similar to the code you are
objecting to.

Doug Gilbert