[PATCH] accel/kvm/kvm-all: Fix wrong return code handling in dirty log code

[PATCH] accel/kvm/kvm-all: Fix wrong return code handling in dirty log code

[Thomas Huth]

The kvm_vm_ioctl() wrapper already returns -errno if the ioctl itself
returned -1, so the callers of kvm_vm_ioctl() should not check for -1
but for a value < 0 instead.

This problem has been fixed once already in commit b533f658a98325d0e4
but that commit missed that the ENOENT error code is not fatal for
this ioctl, so the commit has been reverted in commit 50212d6346f33d6e
since the problem occurred close to a pending release at that point
in time. The plan was to fix it properly after the release, but it
seems like this has been forgotten. So let's do it now finally instead.

Resolves: https://bugs.launchpad.net/qemu/+bug/1294227
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 accel/kvm/kvm-all.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 3feb17d965..7224596932 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -668,16 +668,19 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
 
         d.dirty_bitmap = mem->dirty_bmap;
         d.slot = mem->slot | (kml->as_id << 16);
-        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
-            DPRINTF("ioctl failed %d\n", errno);
-            ret = -1;
+        ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
+        if (ret == -ENOENT) {
+            /* kernel does not have dirty bitmap in this slot */
+            ret = 0;
+        } else if (ret < 0) {
+            error_report("ioctl KVM_GET_DIRTY_LOG failed: %d", errno);
             goto out;
+        } else {
+            subsection.offset_within_region += slot_offset;
+            subsection.size = int128_make64(slot_size);
+            kvm_get_dirty_pages_log_range(&subsection, d.dirty_bitmap);
         }
 
-        subsection.offset_within_region += slot_offset;
-        subsection.size = int128_make64(slot_size);
-        kvm_get_dirty_pages_log_range(&subsection, d.dirty_bitmap);
-
         slot_offset += slot_size;
         start_addr += slot_size;
         size -= slot_size;
@@ -774,8 +777,8 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
     d.num_pages = bmap_npages;
     d.slot = mem->slot | (as_id << 16);
 
-    if (kvm_vm_ioctl(s, KVM_CLEAR_DIRTY_LOG, &d) == -1) {
-        ret = -errno;
+    ret = kvm_vm_ioctl(s, KVM_CLEAR_DIRTY_LOG, &d);
+    if (ret < 0 && ret != -ENOENT) {
         error_report("%s: KVM_CLEAR_DIRTY_LOG failed, slot=%d, "
                      "start=0x%"PRIx64", size=0x%"PRIx32", errno=%d",
                      __func__, d.slot, (uint64_t)d.first_page,
-- 
2.27.0

Re: [PATCH] accel/kvm/kvm-all: Fix wrong return code handling in dirty log code

[Philippe Mathieu-Daudé]

On 1/29/21 9:43 AM, Thomas Huth wrote:
> The kvm_vm_ioctl() wrapper already returns -errno if the ioctl itself
> returned -1, so the callers of kvm_vm_ioctl() should not check for -1
> but for a value < 0 instead.
> 
> This problem has been fixed once already in commit b533f658a98325d0e4
> but that commit missed that the ENOENT error code is not fatal for
> this ioctl, so the commit has been reverted in commit 50212d6346f33d6e
> since the problem occurred close to a pending release at that point
> in time. The plan was to fix it properly after the release, but it
> seems like this has been forgotten. So let's do it now finally instead.
> 
> Resolves: https://bugs.launchpad.net/qemu/+bug/1294227

Is this the "Close the oldest Launchpad bug" contest? =)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)

Re: [PATCH] accel/kvm/kvm-all: Fix wrong return code handling in dirty log code

[Thomas Huth]

On 29/01/2021 10.28, Philippe Mathieu-Daudé wrote:
> On 1/29/21 9:43 AM, Thomas Huth wrote:
>> The kvm_vm_ioctl() wrapper already returns -errno if the ioctl itself
>> returned -1, so the callers of kvm_vm_ioctl() should not check for -1
>> but for a value < 0 instead.
>>
>> This problem has been fixed once already in commit b533f658a98325d0e4
>> but that commit missed that the ENOENT error code is not fatal for
>> this ioctl, so the commit has been reverted in commit 50212d6346f33d6e
>> since the problem occurred close to a pending release at that point
>> in time. The plan was to fix it properly after the release, but it
>> seems like this has been forgotten. So let's do it now finally instead.
>>
>> Resolves: https://bugs.launchpad.net/qemu/+bug/1294227
> 
> Is this the "Close the oldest Launchpad bug" contest? =)

No, in that case I'd had tried to fix 
https://bugs.launchpad.net/qemu/+bug/304636 instead ;-)

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

  Thomas

Re: [PATCH] accel/kvm/kvm-all: Fix wrong return code handling in dirty log code

[Paolo Bonzini]

On 29/01/21 09:43, Thomas Huth wrote:
> The kvm_vm_ioctl() wrapper already returns -errno if the ioctl itself
> returned -1, so the callers of kvm_vm_ioctl() should not check for -1
> but for a value < 0 instead.
> 
> This problem has been fixed once already in commit b533f658a98325d0e4
> but that commit missed that the ENOENT error code is not fatal for
> this ioctl, so the commit has been reverted in commit 50212d6346f33d6e
> since the problem occurred close to a pending release at that point
> in time. The plan was to fix it properly after the release, but it
> seems like this has been forgotten. So let's do it now finally instead.
> 
> Resolves: https://bugs.launchpad.net/qemu/+bug/1294227
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   accel/kvm/kvm-all.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 3feb17d965..7224596932 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -668,16 +668,19 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
>   
>           d.dirty_bitmap = mem->dirty_bmap;
>           d.slot = mem->slot | (kml->as_id << 16);
> -        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
> -            DPRINTF("ioctl failed %d\n", errno);
> -            ret = -1;
> +        ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
> +        if (ret == -ENOENT) {
> +            /* kernel does not have dirty bitmap in this slot */
> +            ret = 0;
> +        } else if (ret < 0) {
> +            error_report("ioctl KVM_GET_DIRTY_LOG failed: %d", errno);
>               goto out;
> +        } else {
> +            subsection.offset_within_region += slot_offset;
> +            subsection.size = int128_make64(slot_size);
> +            kvm_get_dirty_pages_log_range(&subsection, d.dirty_bitmap);
>           }
>   
> -        subsection.offset_within_region += slot_offset;
> -        subsection.size = int128_make64(slot_size);
> -        kvm_get_dirty_pages_log_range(&subsection, d.dirty_bitmap);
> -
>           slot_offset += slot_size;
>           start_addr += slot_size;
>           size -= slot_size;
> @@ -774,8 +777,8 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
>       d.num_pages = bmap_npages;
>       d.slot = mem->slot | (as_id << 16);
>   
> -    if (kvm_vm_ioctl(s, KVM_CLEAR_DIRTY_LOG, &d) == -1) {
> -        ret = -errno;
> +    ret = kvm_vm_ioctl(s, KVM_CLEAR_DIRTY_LOG, &d);
> +    if (ret < 0 && ret != -ENOENT) {
>           error_report("%s: KVM_CLEAR_DIRTY_LOG failed, slot=%d, "
>                        "start=0x%"PRIx64", size=0x%"PRIx32", errno=%d",
>                        __func__, d.slot, (uint64_t)d.first_page,
> 

Queued, thanks.

Paolo