Re: [Qemu-devel] wrong ioctl error handling on dirty pages sync?

[Michael Tokarev]

05.09.2017 14:57, Michael Tokarev пишет:
> 05.09.2017 13:33, Stefan Hajnoczi пишет:
>> On Mon, Sep 04, 2017 at 02:52:57PM +0200, Ján Poctavek wrote:
>>>
>>> On 29. 8. 2017 12:08, Stefan Hajnoczi wrote:
>>>> On Fri, Aug 25, 2017 at 01:42:55PM +0200, Ján Poctavek wrote:
>>>>> Hi guys,
>>>>>
>>>>> Maybe it is just my lack of understanding, this seems like a bug to me:
>>>>>
>>>>> To get list of dirty pages, qemu calls kvm_vm_ioctl() with
>>>>> KVM_GET_DIRTY_LOG:
>>>>> https://github.com/qemu/qemu/blob/v2.10.0-rc4/accel/kvm/kvm-all.c#L494
>>>>>
>>>>> and considers the ioctl call failed when -1 is returned.
>>>>>
>>>>> But the kvm_vm_ioctl() itself returns -errno, not the -1 on error:
>>>>> https://github.com/qemu/qemu/blob/v2.10.0-rc4/accel/kvm/kvm-all.c#L2142
>>>>>
>>>>> Thanks in advance for sheding some light into this for me.
>>>> Looks like a bug to me.  Do you want to send a patch?
>>>>
>>>> Guidelines on how to submit a patch are here:
>>>> http://wiki.qemu.org/Contribute/SubmitAPatch
>>>>
>>>> Stefan
>>>
>>> It seems that the patch has already been created a long time ago. But it is
>>> still not included:
>>>
>>> https://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg03996.html
>>>
>>> Can I help with this somehow?
>>
>> Michael: Do you know what happened to this -trivial patch?
> 
> https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg05347.html
> https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg05346.html
> https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg05402.html
> 
> -- according to the emails, it has been applied, but I don't see it in
> the git tree.  Hmm..

Aha.  Here we go:

commit 50212d6346f33d6e19489e81b025b5c3bb8759be
Author: Michael Tokarev <address@hidden>
Date:   Mon Apr 14 16:14:04 2014 +0400

    Revert "fix return check for KVM_GET_DIRTY_LOG ioctl"

    This reverts commit b533f658a98325d0e47b36113bd9f5bcc046fdae.

    The original code was wrong, because effectively it ignored errors
    from kernel, because kernel does not return -1 on error case but
    returns -errno, and does not return -EPERM for this particular ioctl.
    But in some cases kernel actually returned unsuccessful result,
    namely, when the dirty bitmap in requested slot does not exist
    it returns -ENOENT.  With new code this condition becomes an
    error when it shouldn't be.

    Revert that patch instead of fixing it properly this late in the
    release process.  I disagree with this approach, but let's make
    things move _somewhere_, instead of arguing endlessly whch of
    the 2 proposed fixes is better.

    Signed-off-by: Michael Tokarev <address@hidden>
    Message-id: address@hidden
    Signed-off-by: Peter Maydell <address@hidden>

/mjt