Re: [Qemu-devel] [PATCH v4 04/14] onenand: Switch to byte-based block access

[Eric Blake]

On 05/02/2016 03:16 PM, Eric Blake wrote:
>> First of all, why signed? More importantly, though, sec is an int. I'm
>> not sure if we should cast it to uint64_t before shifting (I'm unsure
>> because this device seems to supports only sizes that fit in a uint32_t
>> anyway), but if we don't, wouldn't it make things more obvious if offset
>> were a uint32_t, too?
> 
> Hmm. I guess sec can't be negative, but I didn't check whether sec can
> ever be greater than 0x7fffffff/BDRV_SECTOR_SIZE.  Depending on that
> answer determines whether the shift here overflows - but you are right
> that if it CAN overflow 32 bits, then we MUST cast sec to a 64-bit type
> PRIOR to the shift, not just merely assign it to a 64-bit value; and if
> CAN'T overflow, then a 32-bit type is sufficient to hold the answer.
> You're also right that unsigned is nicer in general for sizes that
> shouldn't be negative.

Okay, auditing wasn't as hard as I feared:

static int onenand_initfn(SysBusDevice *sbd)
...
    uint32_t size = 1 << (24 + ((s->id.dev >> 4) & 7));
...
    s->blocks = size >> BLOCK_SHIFT;
    s->secs = size >> 9;

so the maximum sec should ever be is 0x80000000 >> 9. I'll stick with
uint32_t (since that's what the init function used), and maybe add an
assert that we aren't overflowing.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature