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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

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 <eblake@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]