[Qemu-devel] Re: [PATCH] kvm: Fix error detection for KVM_GET_DIRTY_LOG

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

Alexander Graf wrote:
> 
> 
> 
> 
> Am 22.07.2009 um 22:07 schrieb Jan Kiszka <jan.kiszka@web.de>:
> 
>> Anthony Liguori wrote:
>>> Jan Kiszka wrote:
>>>>> That makes me uncomfortable.  Shouldn't we make kvm return something
>>>>> that's exposed to userspace?
>>>>>
>>>>>
>>>>
>>>> Yes, but we can't do this from user space :) (or in other words: there
>>>> are already kernels out there which return this invalid code).
>>>>
>>>
>>> But since it's not symbolic, at some point in time the meaning of 524
>>> can potentially change and introduce a very, very subtle bug.
>>
>> I won't change in old kernel versions, and I expect someone from the
>> PowerPC folks to fix it for new version fairly soon.
>>
>>>
>>>> The situation would only be different if Alex said that it takes
>>>> further
>>>> kernel patches anyway to make his PowerPC targets work. Dunno.
>>>>
>>>
>>> Certainly, his PPC target is not in any released kernel version so
>>> there's time to fix things properly.
>>
>> Ah, ok. Then let's do this (Alex can carry a temporary workaround
>> locally IMHO):
>>
>> ------------->
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> kvm-all.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 824bb4c..5fb8dba 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -348,7 +348,7 @@ int
>> kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>         d.slot = mem->slot;
>>
>>         r = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
>> -        if (r == -EINVAL) {
>> +        if (r < 0) {
> 
> That's worse as it essentially removes the not implemented case.

You can add the valid return code of a fixed powerpc version of
kvm_vm_ioctl_get_dirty_log here.

> 
> The only thing I'd agree with as a good idea instead of the version I
> sent is an explicit switch statement with an include of errno.h with
> __KERNEL__ set.

Sorry, that's also horrible.

Again the question: Is there already some official, unpatched kernel
version which would work with QEMU if we only handle that borken
ENOTSUPP case?

If not, I see no point in handling it here. Please fix KVM and provide a
patch that checks for the fixed, valid error code.

If there is such a kernel, I see no problem with hard-coding ENOTSUPP's
value now, as we are working around a hard-coded bug that will no longer
exist in the future (where ENOTSUPP may change its value - theoretically).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]