On 05/02/2016 09:35 AM, Kevin Wolf wrote: > Am 29.04.2016 um 22:08 hat Eric Blake geschrieben: >> Sector-based blk_write() should die; switch to byte-based >> blk_pwrite() instead. Likewise for blk_read(). >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >> >> --- >> Not compile tested - I'm not sure what else I'd need in my environment >> to actually test this one. I have: >> Fedora 23, dnf builddep qemu >> ./configure --enable-kvm --enable-system --disable-user >> --target-list=x86_64-softmmu,ppc64-softmmu --enable-debug >> --- >> hw/block/onenand.c | 36 ++++++++++++++++++++++-------------- >> 1 file changed, 22 insertions(+), 14 deletions(-) >>
>> @@ -257,19 +259,20 @@ static inline int onenand_prog_main(OneNANDState *s,
>> int sec, int secn,
>> int result = 0;
>>
>> if (secn > 0) {
>> - uint32_t size = (uint32_t)secn * 512;
>> + uint32_t size = (uint32_t)secn << BDRV_SECTOR_BITS;
>> + int64_t offset = sec << BDRV_SECTOR_BITS;
>
> I'm not completely happy with the types here.
>
> 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.
>
> And if we decide for casting, there are more places in this patch where
> an int is shifted.
Good catch. I guess I have to audit things more closely before
respinning the series.
>> @@ -295,7 +298,8 @@ static inline int onenand_load_spare(OneNANDState *s,
>> int sec, int secn,
>> uint8_t buf[512];
>>
>> if (s->blk_cur) {
>> - if (blk_read(s->blk_cur, s->secs_cur + (sec >> 5), buf, 1) < 0) {
>> + int32_t offset = (s->secs_cur + (sec >> 5)) << BDRV_SECTOR_BITS;
>
> Here you have 32 bits (though still signed). In any case, some
> consistency couldn't hurt.
That's certainly true, no matter what type I ultimately pick.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
