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
