Re: [Qemu-devel] [PATCH 3/6] Fix last page errors in page_set_flags and page_check_range.

[Richard Henderson <rth@twiddle.net>]

On 02/12/2010 11:47 AM, Blue Swirl wrote:
> Please make separate patches for unrelated changes. Now the essence of
> the patch is very hard to see. Also pure formatting changes are not
> very useful.

Is this just about page_get_flags, or was there some other pure 
formatting change to which you object?  For instance:

>> -    if (start + len < start)
>> -        /* we've wrapped around */
>> +    if (start + len - 1 < start) {
>> +        /* We've wrapped around.  */
>>         return -1;
>> +    }

Only the first line is required to fix an off-by-one error.  But if I 
don't add the braces, generally the patch will be bounced for that.

>> -        /* We may be called for host regions that are outside guest
>> -           address space.  */
>
> Why remove the comment, is it no longer true?

Yes, after the entire patch series is applied.  Indeed, I believe that 
many of the addresses rejected here were *not* in fact outside the guest 
address space, merely that page_find_alloc did not accurately know what 
the guest address space was.

I think it was a mistake to ever consider usage of this function with 
out-of-band addresses as valid -- it adds a great deal of confusion.

I could add an assertion here to make sure, if you like.

If this were C++, it might have been interesting to try to use the type 
system to keep the host and guest address spaces forcibly separate.  But 
since this is plain C, I think wrapping everything in structures and 
access macros would just be too ugly.


r~