On Fri, Feb 12, 2010 at 10:16 PM, Richard Henderson <r...@twiddle.net> 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~
>
