Re: [PATCH 0/9] QEMU file cleanups

[Juan Quintela <quintela@redhat.com>]

Peter Xu <peterx@redhat.com> wrote:
> On Thu, May 04, 2023 at 04:56:46PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote:
>> >> - convince and review code to see that everything is uint64_t.
>> >
>> > One general question to patches regarding this - what's the major benefit
>> > of using uint64_t?
>> >
>> > It doubles the possible numbers to hold, but it's already 64bits so I don't
>> > think it matters a lot.
>> 
>> We were checking for negatives even when that can't be.
>> And we are doing this dance of
>> 
>> int64_t x, y;
>> uint64_t a, b;
>> 
>> x = a;
>> b = y;
>> 
>> This is always confusing and not always right.
>
> Yeah this is confusing, but if anything can go wrong with this I assume we
> could have some bigger problem anyway..
>
>> 
>> > The thing is we're removing some code trying to
>> > detect negative which seems to be still helpful to detect e.g. overflows
>> > (even though I don't think it'll happen).  I just still think it's good to
>> > know when overflow happens, and not sure what I missed on benefits of using
>> > unsigned here.
>> 
>> If you grep through the code, you see that half of the things are
>> int64_t and the other half is uint64_t.  I find it always confusing.
>
> Right, I'm personally curious whether we should just use int64_t always
> unless necessary. :) Another good thing with int64_t is it's also suitable
> for error report when used in retvals.

It is used by sizes.  We don't return errors right now.
And if it is negative, we need to check that if it is negative, even
when no code uses the negative functionality.

> But no strong opinion here, I don't think that's a huge deal for now.
> Having such an alignment on types makes sense to me.

Thanks, Juan.