Glauber Costa wrote:
> On Wed, Sep 10, 2008 at 09:23:18PM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> From: Glauber Costa <glommer@redhat.com>
>>>
>>> kvm_cpu_register_physical_memory() is its only user. Remove it.
>>>
>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>> ---
>>>  qemu/qemu-kvm.c |   52 +++++++++++++++++++++-------------------------------
>>>  1 files changed, 21 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
>>> index 8d366e5..f0ef21e 100644
>>> --- a/qemu/qemu-kvm.c
>>> +++ b/qemu/qemu-kvm.c
>>> @@ -775,42 +775,32 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
>>>                                        unsigned long size,
>>>                                        unsigned long phys_offset)
>>>  {
>>> -#ifdef KVM_CAP_USER_MEMORY
>>>      int r = 0;
>>> -
>>> -    r = kvm_check_extension(kvm_context, KVM_CAP_USER_MEMORY);
>>> -    if (r) {
>>> -        if (!(phys_offset & ~TARGET_PAGE_MASK)) {
>>> -                r = kvm_is_allocated_mem(kvm_context, start_addr, size);
>>> -            if (r)
>>> -                return;
>>> -            r = kvm_is_intersecting_mem(kvm_context, start_addr);
>>> -            if (r)
>>> -                kvm_create_mem_hole(kvm_context, start_addr, size);
>>> -            r = kvm_register_userspace_phys_mem(kvm_context, start_addr,
>>> -                                                phys_ram_base + phys_offset,
>>> -                                                size, 0);
>>> -        }
>>> -        if (phys_offset & IO_MEM_ROM) {
>>> -            phys_offset &= ~IO_MEM_ROM;
>>> -            r = kvm_is_intersecting_mem(kvm_context, start_addr);
>>> -            if (r)
>>> -                kvm_create_mem_hole(kvm_context, start_addr, size);
>>> -            r = kvm_register_userspace_phys_mem(kvm_context, start_addr,
>>> -                                                phys_ram_base + phys_offset,
>>> -                                                size, 0);
>>> -        }
>>> -        if (r < 0) {
>>> -            printf("kvm_cpu_register_physical_memory: failed\n");
>>> -            exit(1);
>>> -        }
>>> -        return;
>>> +    if (!(phys_offset & ~TARGET_PAGE_MASK)) {
>>> +        r = kvm_is_allocated_mem(kvm_context, start_addr, size);
>>> +        if (r)
>>> +            return;
>>> +        r = kvm_is_intersecting_mem(kvm_context, start_addr);
>>> +        if (r)
>>> +            kvm_create_mem_hole(kvm_context, start_addr, size);
>>> +        r = kvm_register_userspace_phys_mem(kvm_context, start_addr,
>>> +                                            phys_ram_base + phys_offset,
>>> +                                            size, 0);
>>>      }
>>> -#endif
>>>      if (phys_offset & IO_MEM_ROM) {
>> At this chance: Shouldn't this become 'else if'?
> 
> fyi: just sent out a new series, this time with numbers and no tabs in qemu ;-)

Yeah, much better. :)

> 
> As a coding style practice, yes. But they are mutually exclusive anyway, so it shouldn't
> matter. We could definitely try to make it better, but they way to go is to have only one case
> instead of two: look at how close they are to each other! I have a later series that do that,
> so I'm just trying to set up the ground here.

Well, they are close partly because the IO_MEM_ROM part is incomplete
(it ignores the 'read-only' property).

However, I just stumbled over it while trying to understand how this
function works. Without the 'else', I had to check twice that the cases
are exclusive in practice...

Jan