Re: [Qemu-devel] Re: [COMMIT bd83677] Fake dirty loggin when it's not there

[Anthony Liguori <anthony@codemonkey.ws>]

Jan Kiszka wrote:
>> Signed-off-by: Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org>
>> Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 961fa32..2032949 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -300,6 +300,7 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>      KVMDirtyLog d;
>>      KVMSlot *mem;
>>      int ret = 0;
>> +    int r;
>>  
>>      d.dirty_bitmap = NULL;
>>      while (start_addr < end_addr) {
>> @@ -308,6 +309,11 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>              break;
>>          }
>>  
>> +        /* We didn't activate dirty logging? Don't care then. */
>> +        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
>> +            continue;
>> +        }
>> +
>>     
>
> According to Alex' reply [1], I think this is probably better expressed
> as an assert() than a silent skip. It indicates an error at higher
> level, no?
>   

I don't have a strong opinion either way.

>>          size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
>>          if (!d.dirty_bitmap) {
>>              d.dirty_bitmap = qemu_malloc(size);
>> @@ -319,7 +325,8 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>  
>>          d.slot = mem->slot;
>>  
>> -        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
>> +        r = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
>> +        if (r == -EINVAL) {
>>              dprintf("ioctl failed %d\n", errno);
>>              ret = -1;
>>              break;
>>     
>
> My remark still stands: This is an "optimistic" assumption that only
> EINVAL is a "real" error. If KVM is buggy /wrt a specific invalid return
> value, work around that particular bug (and we should also fix the
> kernel, but that's a different topic).
>   

The behavior is identical to what it was before.  It's just a stylistic 
change.

I agree that this needs to be properly fixed in KVM and then in QEMU but 
I also think that that is best done as a separate set of patches.

Regards,

Anthony Liguori