Re: [Qemu-devel] [PATCH for-2.0] kvm_physical_sync_dirty_bitmap: ignore ENOENT from kvm_vm_ioctl

[Serge E. Hallyn]

Quoting Paolo Bonzini (address@hidden):
> Il 11/04/2014 22:10, Serge Hallyn ha scritto:
> >Quoting Paolo Bonzini (address@hidden):
> >>Il 11/04/2014 04:31, Michael Tokarev ha scritto:
> >>>ENOENT means the kernel has an empty dirty bitmap for this
> >>>slot.  Don't abort in that case.  This appears to solve
> >>>the bug reported at
> >>>
> >>>https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1303926
> >>>
> >>>which first showed up with commit b533f658a98325d: fix return
> >>>check for KVM_GET_DIRTY_LOG ioctl
> >>>
> >>>Cc: Serge Hallyn <address@hidden>
> >>>Signed-off-by: Michael Tokarev <address@hidden>
> >>>---
> >>>kvm-all.c |   10 ++++++++--
> >>>1 file changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git a/kvm-all.c b/kvm-all.c
> >>>index cd4111d..47fa948 100644
> >>>--- a/kvm-all.c
> >>>+++ b/kvm-all.c
> >>>@@ -441,13 +441,19 @@ static int 
> >>>kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section)
> >>>
> >>>        d.slot = mem->slot;
> >>>
> >>>-        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) < 0) {
> >>>+        ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
> >>>+        if (ret >= 0) {
> >>>+            /* regular case, process returned bitmap */
> >>>+            kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
> >>>+        } else if (ret == -ENOENT) {
> >>>+            /* kernel does not have dirty bitmap in this slot */
> >>>+            ret = 0;
> >>>+        } else {
> >>>            DPRINTF("ioctl failed %d\n", errno);
> >>>            ret = -1;
> >>>            break;
> >>>        }
> >>>
> >>>-        kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
> >>>        start_addr = mem->start_addr + mem->memory_size;
> >>>    }
> >>>    g_free(d.dirty_bitmap);
> >>>
> >>
> >>I'd rather revert b533f658a98325d instead.
> >
> >That seems wrong though.  If we want to ignore all errors that's one
> >thing, but before that commit we just ignored all errors other than
> >EPERM.
> 
> It is wrong, and the patch would get in again for 2.1 via the KVM
> tree.  But if the patch caused a regression it obviously wasn't
> trivial enough, and so close to 2.0 the safest thing to do is revert
> it.

True it seemed all right before that commit.  Though if it wasn't all right
we'd see the error a long ways down the road.  The only other errors we
expect from the ioctl are EFAULT and EINVAL.  EINVAL would mean we fed
it bad data (log->slot >= KVM_USER_MEM_SLOTS), while EFAULT would mean
we passed a bad address for the returned bitmap.  The latter one probably
shouldn't be ignored - however it's not as though we do anything in the error
case to try and recover.

So given that the code has been as it was since at least v1.3.0, perhaps
a revert is the most responsible thing to do.

-serge