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

[Anthony Liguori]

Jan Kiszka wrote:
> > Signed-off-by: Alexander Graf <address@hidden>
> > Signed-off-by: Anthony Liguori <aliguori-r/address@hidden>
> >
> > 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