Re: [PATCH 3/9] allow intersecting region to be on the boundary.

[Avi Kivity <avi@redhat.com>]

Glauber Costa wrote:
> On Sat, Sep 20, 2008 at 11:32:48AM -0700, Avi Kivity wrote:
>   
>> Glauber Costa wrote:
>>     
>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>> ---
>>>  libkvm/libkvm.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
>>> index e768e44..fa65c30 100644
>>> --- a/libkvm/libkvm.c
>>> +++ b/libkvm/libkvm.c
>>> @@ -130,8 +130,8 @@ int get_intersecting_slot(unsigned long phys_addr)
>>>  	int i;
>>>   	for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS ; ++i)
>>> -		if (slots[i].len && slots[i].phys_addr < phys_addr &&
>>> -		    (slots[i].phys_addr + slots[i].len) > phys_addr)
>>> +		if (slots[i].len && slots[i].phys_addr <= phys_addr &&
>>> +		    (slots[i].phys_addr + slots[i].len) >= phys_addr)
>>>  			return i;
>>>  	return -1;
>>>   
>>>       
>> consider
>>
>>  slots[i].phys_addr = 0
>>  slots[i].len = 1
>>  phys_addr = 1
>>
>> with the new calculation, i (well, not me personally) will be considered  
>> an intersecting slot.
>>
>> Not that I (me this time) can understand how you can calculate interval  
>> intersection without the entire interval.
>>     
> would you be fine with checking only the left interval?
>
>   

You mean the left edge?  That's what we're doing now.  No checking or 
complete checking are understandable, but partial checking seems an 
invitation for something to break.

> But to be honest, look at:
>
>     r = kvm_is_containing_region(kvm_context, start_addr, size);
>     if (r)
>         return;
>
>     [ sip ]
>
>     r = kvm_is_intersecting_mem(kvm_context, start_addr);
>     if (r) {
>         printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size);
>
> We don't really do anything, which is the same action as the containing region case.
> So maybe we should just merge the two checks, and do the same thing (nothing) on both?
>   

Yes.  So long as it's self-consistent, which the current code (before 
your patches) isn't.

Does no one read this code before merging it?!

-- 
error compiling committee.c: too many arguments to function