[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v4 04/14] onenand: Switch to byte-based block access
Date: Mon, 2 May 2016 15:29:41 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

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

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]