[Qemu-devel] [PATCH 0/2] win32-aio fixes

[Qemu-devel] [PATCH 0/2] win32-aio fixes

[Kevin Wolf]

Paolo, especially the first one is worrying with respect to the test status of
this code. We should probably give it some additional testing.

Kevin Wolf (2):
  win32-aio: Fix vectored reads
  win32-aio: Fix memory leak

 block/win32-aio.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH 1/2] win32-aio: Fix vectored reads

[Kevin Wolf]

Copying data in the right direction really helps a lot!

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/win32-aio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/win32-aio.c b/block/win32-aio.c
index 46a5db7..53b82e6 100644
--- a/block/win32-aio.c
+++ b/block/win32-aio.c
@@ -84,7 +84,7 @@ static void win32_aio_process_completion(QEMUWin32AIOState *s,
             int i;
 
             for (i = 0; i < qiov->niov; ++i) {
-                memcpy(p, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
+                memcpy(qiov->iov[i].iov_base, p, qiov->iov[i].iov_len);
                 p += qiov->iov[i].iov_len;
             }
             g_free(waiocb->buf);
-- 
1.7.6.5

Re: [Qemu-devel] [Qemu-stable] [PATCH 1/2] win32-aio: Fix vectored reads

[Michael Tokarev]

17.01.2013 00:19, Kevin Wolf пишет:
> Copying data in the right direction really helps a lot!
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/win32-aio.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/block/win32-aio.c b/block/win32-aio.c
> index 46a5db7..53b82e6 100644
> --- a/block/win32-aio.c
> +++ b/block/win32-aio.c
> @@ -84,7 +84,7 @@ static void win32_aio_process_completion(QEMUWin32AIOState *s,
>               int i;
>
>               for (i = 0; i < qiov->niov; ++i) {
> -                memcpy(p, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
> +                memcpy(qiov->iov[i].iov_base, p, qiov->iov[i].iov_len);
>                   p += qiov->iov[i].iov_len;
>               }
>               g_free(waiocb->buf);

Actually this is just

    iov_from_buf(qiov->iov, qiov->niov, 0, waiocb->buf, -1);

Or is it iov_to_buf() ? :)

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-stable] [PATCH 1/2] win32-aio: Fix vectored reads

[Kevin Wolf]

Am 17.01.2013 09:33, schrieb Michael Tokarev:
> 17.01.2013 00:19, Kevin Wolf пишет:
>> Copying data in the right direction really helps a lot!
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/win32-aio.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/block/win32-aio.c b/block/win32-aio.c
>> index 46a5db7..53b82e6 100644
>> --- a/block/win32-aio.c
>> +++ b/block/win32-aio.c
>> @@ -84,7 +84,7 @@ static void win32_aio_process_completion(QEMUWin32AIOState *s,
>>               int i;
>>
>>               for (i = 0; i < qiov->niov; ++i) {
>> -                memcpy(p, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
>> +                memcpy(qiov->iov[i].iov_base, p, qiov->iov[i].iov_len);
>>                   p += qiov->iov[i].iov_len;
>>               }
>>               g_free(waiocb->buf);
> 
> Actually this is just
> 
>     iov_from_buf(qiov->iov, qiov->niov, 0, waiocb->buf, -1);

True. Let's keep fix and cleanup separate, though. Feel free to send a
cleanup patch on top of this.

By the way, I think qiov->size instead of -1 would be nicer.

> Or is it iov_to_buf() ? :)

The corrected version is from.

Kevin

[Qemu-devel] [PATCH 2/2] win32-aio: Fix memory leak

[Kevin Wolf]

The buffer is allocated for both reads and writes, and obviously it
should be freed even if an error occurs.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/win32-aio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/block/win32-aio.c b/block/win32-aio.c
index 53b82e6..546a60d 100644
--- a/block/win32-aio.c
+++ b/block/win32-aio.c
@@ -87,8 +87,8 @@ static void win32_aio_process_completion(QEMUWin32AIOState *s,
                 memcpy(qiov->iov[i].iov_base, p, qiov->iov[i].iov_len);
                 p += qiov->iov[i].iov_len;
             }
-            g_free(waiocb->buf);
         }
+        g_free(waiocb->buf);
     }
 
 
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH 0/2] win32-aio fixes

[Stefan Hajnoczi]

On Wed, Jan 16, 2013 at 09:19:58PM +0100, Kevin Wolf wrote:
> Paolo, especially the first one is worrying with respect to the test status of
> this code. We should probably give it some additional testing.
> 
> Kevin Wolf (2):
>   win32-aio: Fix vectored reads
>   win32-aio: Fix memory leak
> 
>  block/win32-aio.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> -- 
> 1.7.6.5
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [PATCH 0/2] win32-aio fixes

[Paolo Bonzini]

Il 16/01/2013 21:19, Kevin Wolf ha scritto:
> Paolo, especially the first one is worrying with respect to the test status of
> this code. We should probably give it some additional testing.
> 
> Kevin Wolf (2):
>   win32-aio: Fix vectored reads
>   win32-aio: Fix memory leak
> 
>  block/win32-aio.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 

Yes, the worrying part is especially that IIUC qtest does not support
Windows.  There's no way to get good coverage without qtest.

Paolo

Re: [Qemu-devel] [PATCH 0/2] win32-aio fixes

[Kevin Wolf]

Am 17.01.2013 11:53, schrieb Paolo Bonzini:
> Il 16/01/2013 21:19, Kevin Wolf ha scritto:
>> Paolo, especially the first one is worrying with respect to the test status of
>> this code. We should probably give it some additional testing.
>>
>> Kevin Wolf (2):
>>   win32-aio: Fix vectored reads
>>   win32-aio: Fix memory leak
>>
>>  block/win32-aio.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
> 
> Yes, the worrying part is especially that IIUC qtest does not support
> Windows.  There's no way to get good coverage without qtest.

Why that? If block drivers aren't testable from qemu-iotests with only
qemu-img and qemu-io, then there's something we did wrong. This specific
code would have been easily covered with qemu-io -k -n -c 'readv ...'
(which is how I found the bug and tested the fix).

Hm, or actually, is cache=none even needed for aio=native on Windows? In
any case I think some documentation needs to be updated.

qemu-iotests under Wine may need some polishing, though, and of course
needs someone to run it regularly with the right parameters. (In fact,
it seems we don't even run the tests with Linux AIO)

Kevin

Re: [Qemu-devel] [PATCH 0/2] win32-aio fixes

[Paolo Bonzini]

Il 17/01/2013 12:35, Kevin Wolf ha scritto:
> Am 17.01.2013 11:53, schrieb Paolo Bonzini:
>> Il 16/01/2013 21:19, Kevin Wolf ha scritto:
>>> Paolo, especially the first one is worrying with respect to the test status of
>>> this code. We should probably give it some additional testing.
>>>
>>> Kevin Wolf (2):
>>>   win32-aio: Fix vectored reads
>>>   win32-aio: Fix memory leak
>>>
>>>  block/win32-aio.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>
>> Yes, the worrying part is especially that IIUC qtest does not support
>> Windows.  There's no way to get good coverage without qtest.
> 
> Why that? If block drivers aren't testable from qemu-iotests with only
> qemu-img and qemu-io, then there's something we did wrong. This specific
> code would have been easily covered with qemu-io -k -n -c 'readv ...'
> (which is how I found the bug and tested the fix).

Doh, that was really stupid.

/me unsuccessfully tries to blame flu

> Hm, or actually, is cache=none even needed for aio=native on Windows? In
> any case I think some documentation needs to be updated.

Honestly I have no idea.  However, I don't think so.

Paolo

> qemu-iotests under Wine may need some polishing, though, and of course
> needs someone to run it regularly with the right parameters. (In fact,
> it seems we don't even run the tests with Linux AIO)