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

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

[Ján Poctavek]

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.

Jan

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

[Stefan Hajnoczi]

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

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

[Ján Poctavek]

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?

Jan

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

[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?

Thanks,
Stefan

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

[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..

Peter&

/mjt

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 <mjt@tls.msk.ru>
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 <mjt@tls.msk.ru>
    Message-id: 1397477644-902-1-git-send-email-mjt@msgid.tls.msk.ru
    Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

/mjt