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

[Blue Swirl]

On Fri, Feb 12, 2010 at 10:16 PM, Richard Henderson <address@hidden> wrote:
> 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:

Also this one:
-    end = TARGET_PAGE_ALIGN(start+len); /* must do before we loose
bits in the next step */
+    /* END must be computed before we drop bits from START.  */
+    end = TARGET_PAGE_ALIGN(start + len);
    start = start & TARGET_PAGE_MASK;

and these:

-        if( !p )
+        if (!p)
            return -1;
-        if( !(p->flags & PAGE_VALID) )
+        if (!(p->flags & PAGE_VALID))

>
>>> -    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.

That change is OK.

>>> -        /* 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~
>