Re: [Qemu-devel] [PATCH] bitops: fix types

[Blue Swirl]

On Sun, Jul 8, 2012 at 2:04 PM, Peter Maydell <address@hidden> wrote:
> On 8 July 2012 13:12,  <address@hidden> wrote:
>> -static inline uint64_t deposit64(uint64_t value, int start, int length,
>> -                                 uint64_t fieldval)
>> +static inline uint64_t deposit64(uint64_t value, unsigned int start,
>> +                                 unsigned int length, uint64_t fieldval)
>>  {
>>      uint64_t mask;
>> -    assert(start >= 0 && length > 0 && length <= 64 - start);
>> +    assert(length > 0 && length <= 64 - start);
>
> This breaks the assertion (consider the case of start == UINT_MAX
> and length == 64).

The original is equally buggy in other cases since there is no bound
check for the upper limit. I think this should catch all:

assert(start < 64 && length > 0 && length <= 64  && start + length  <= 64);

>
> In general, this patch is fixing something that isn't broken
> and pointlessly diverging from the well-tested kernel versions
> of these functions.

This is not pointless. We are using plenty of other things from
various sources and there is very little effort to keep things
identical to originals (except for KVM kernel headers).

> It's also trying to do several things at once
> (eg "drop volatile" is probably less controversial.)

I agree.

>
> -- PMM