win32-aio fixes

PATCH Applied Kevin Wolf 4 participants 9 messages Jan 16, 2013 – Jan 17, 2013 Block layer

Share & tools

Kevin Wolf

author

2013-01-16 20:53 UTC #0 lore raw

cover win32-aio fixes

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(-)

Signature · 1 line

1.7.6.5

Kevin Wolf

author

2013-01-16 20:53 UTC #1 lore raw

PATCH 1/2 win32-aio: Fix vectored reads

Copying data in the right direction really helps a lot!

Cc [email protected]

Signed-off-by Kevin Wolf <[email protected]>

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

block/win32-aio.c +1 -1

index 46a5db7..53b82e6 100644
@@ -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);

Signature · 1 line

1.7.6.5

Kevin Wolf

author

2013-01-17 08:43 UTC #3 lore raw

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

Am 17.01.2013 09:33, schrieb Michael Tokarev:

Quoted 25 lines

17.01.2013 00:19, Kevin Wolf пишет:
Quoted 13 lines
Copying data in the right direction really helps a lot!
Cc [email protected]
Signed-off-by Kevin Wolf <[email protected]>
---
  block/win32-aio.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
block/win32-aio.c +0 -0
index 46a5db7..53b82e6 100644
block/win32-aio.c static void win32_aio_process_completion(QEMUWin32AIOState *s, · line 90
Quoted 8 lines
block/win32-aio.c +1 -1
@@ -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

Quoted 1 line

    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.

Quoted 1 line

Or is it iov_to_buf() ? :)

The corrected version is from.

Kevin

Kevin Wolf

author

2013-01-16 20:53 UTC #4 lore raw

PATCH 2/2 win32-aio: Fix memory leak

In reply to Kevin Wolf

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

Cc [email protected]

Signed-off-by Kevin Wolf <[email protected]>

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

block/win32-aio.c +1 -1

index 53b82e6..546a60d 100644
@@ -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);
     }

Signature · 1 line

1.7.6.5

Stefan Hajnoczi

Applied

2013-01-17 09:59 UTC #5 lore raw

In reply to Kevin Wolf

Quoted 13 lines · 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(-)
Signature · 1 line
1.7.6.5

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

Stefan

Paolo Bonzini

2013-01-17 12:37 UTC #6 lore raw

In reply to Kevin Wolf

Il 16/01/2013 21:19, Kevin Wolf ha scritto:

Quoted 10 lines

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

Kevin Wolf

author

2013-01-17 11:35 UTC #7 lore raw

Am 17.01.2013 11:53, schrieb Paolo Bonzini:

Quoted 8 lines

Il 16/01/2013 21:19, Kevin Wolf ha scritto:
Quoted 7 lines
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

Quoted 6 lines

Quoted 3 lines
 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

Paolo Bonzini

2013-01-17 11:37 UTC #8 lore raw

Il 17/01/2013 12:35, Kevin Wolf ha scritto:

Quoted 13 lines

Am 17.01.2013 11:53, schrieb Paolo Bonzini:
Quoted 12 lines
Il 16/01/2013 21:19, Kevin Wolf ha scritto:
Quoted 10 lines
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(-)

Quoted 7 lines

Quoted 2 lines
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

Quoted 2 lines

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

Quoted 3 lines

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)