[Qemu-devel] [PATCH 0/4] Fixes around qemu_vfree()

[Qemu-devel] [PATCH 0/4] Fixes around qemu_vfree()

[Markus Armbruster]

Markus Armbruster (4):
  w32: Make qemu_vfree() accept NULL like the POSIX implementation
  scsi-disk: qemu_vfree(NULL) is fine, simplify
  win32-aio: Fix how win32_aio_process_completion() frees buffer
  block: Fix how mirror_run() frees its buffer

 block/mirror.c     | 2 +-
 block/win32-aio.c  | 2 +-
 hw/scsi-disk.c     | 4 +---
 util/oslib-win32.c | 4 +++-
 4 files changed, 6 insertions(+), 6 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/4] w32: Make qemu_vfree() accept NULL like the POSIX implementation

[Markus Armbruster]

On POSIX, qemu_vfree() accepts NULL, because it's merely wrapper
around free().  As far as I can tell, the Windows implementation
doesn't.  Breeds bugs that bite only under Windows.

Make the Windows implementation behave like the POSIX implementation.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/oslib-win32.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index e7e283e..640194c 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -71,7 +71,9 @@ void *qemu_vmalloc(size_t size)
 void qemu_vfree(void *ptr)
 {
     trace_qemu_vfree(ptr);
-    VirtualFree(ptr, 0, MEM_RELEASE);
+    if (ptr) {
+        VirtualFree(ptr, 0, MEM_RELEASE);
+    }
 }
 
 /* FIXME: add proper locking */
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/4] scsi-disk: qemu_vfree(NULL) is fine, simplify

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi-disk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index f8d7ef3..96db9a7 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -85,9 +85,7 @@ static void scsi_free_request(SCSIRequest *req)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
 
-    if (r->iov.iov_base) {
-        qemu_vfree(r->iov.iov_base);
-    }
+    qemu_vfree(r->iov.iov_base);
 }
 
 /* Helper function for command completion with sense.  */
-- 
1.7.11.7

[Qemu-devel] [PATCH 3/4] win32-aio: Fix how win32_aio_process_completion() frees buffer

[Markus Armbruster]

win32_aio_submit() allocates it with qemu_blockalign(), therefore it
must be freed with qemu_vfree(), not g_free().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/win32-aio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/win32-aio.c b/block/win32-aio.c
index 46a5db7..0383370 100644
--- a/block/win32-aio.c
+++ b/block/win32-aio.c
@@ -87,7 +87,7 @@ static void win32_aio_process_completion(QEMUWin32AIOState *s,
                 memcpy(p, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
                 p += qiov->iov[i].iov_len;
             }
-            g_free(waiocb->buf);
+            qemu_vfree(waiocb->buf);
         }
     }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 4/4] block: Fix how mirror_run() frees its buffer

[Markus Armbruster]

It allocates with qemu_blockalign(), therefore it must free with
qemu_vfree(), not g_free().

Since I'm touching it anyway, move the free to a more obviosly correct
place.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/mirror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 8aeacbf..27a7d8c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -223,9 +223,9 @@ static void coroutine_fn mirror_run(void *opaque)
             break;
         }
     }
+    qemu_vfree(s->buf);
 
 immediate_exit:
-    g_free(s->buf);
     bdrv_set_dirty_tracking(bs, false);
     bdrv_iostatus_disable(s->target);
     if (s->should_complete && ret == 0) {
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 3/4] win32-aio: Fix how win32_aio_process_completion() frees buffer

[Kevin Wolf]

Am 15.01.2013 14:23, schrieb Markus Armbruster:
> win32_aio_submit() allocates it with qemu_blockalign(), therefore it
> must be freed with qemu_vfree(), not g_free().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/win32-aio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/win32-aio.c b/block/win32-aio.c
> index 46a5db7..0383370 100644
> --- a/block/win32-aio.c
> +++ b/block/win32-aio.c
> @@ -87,7 +87,7 @@ static void win32_aio_process_completion(QEMUWin32AIOState *s,
>                  memcpy(p, qiov->iov[i].iov_base, qiov->iov[i].iov_len);
>                  p += qiov->iov[i].iov_len;
>              }
> -            g_free(waiocb->buf);
> +            qemu_vfree(waiocb->buf);
>          }
>      }

Independent bug: waiocb->buf is leaked for writes and failed reads.

Kevin

Re: [Qemu-devel] [PATCH 4/4] block: Fix how mirror_run() frees its buffer

[Kevin Wolf]

Am 15.01.2013 14:23, schrieb Markus Armbruster:
> It allocates with qemu_blockalign(), therefore it must free with
> qemu_vfree(), not g_free().
> 
> Since I'm touching it anyway, move the free to a more obviosly correct
> place.

...except that it's now leaked for all error cases but the first.

Kevin

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 8aeacbf..27a7d8c 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -223,9 +223,9 @@ static void coroutine_fn mirror_run(void *opaque)
>              break;
>          }
>      }
> +    qemu_vfree(s->buf);
>  
>  immediate_exit:
> -    g_free(s->buf);
>      bdrv_set_dirty_tracking(bs, false);
>      bdrv_iostatus_disable(s->target);
>      if (s->should_complete && ret == 0) {
> 

Re: [Qemu-devel] [PATCH 0/4] Fixes around qemu_vfree()

[Kevin Wolf]

Am 15.01.2013 14:23, schrieb Markus Armbruster:
> Markus Armbruster (4):
>   w32: Make qemu_vfree() accept NULL like the POSIX implementation
>   scsi-disk: qemu_vfree(NULL) is fine, simplify
>   win32-aio: Fix how win32_aio_process_completion() frees buffer
>   block: Fix how mirror_run() frees its buffer
> 
>  block/mirror.c     | 2 +-
>  block/win32-aio.c  | 2 +-
>  hw/scsi-disk.c     | 4 +---
>  util/oslib-win32.c | 4 +++-
>  4 files changed, 6 insertions(+), 6 deletions(-)

Patches 1-3:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 4/4] block: Fix how mirror_run() frees its buffer

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 15.01.2013 14:23, schrieb Markus Armbruster:
>> It allocates with qemu_blockalign(), therefore it must free with
>> qemu_vfree(), not g_free().
>> 
>> Since I'm touching it anyway, move the free to a more obviosly correct
>> place.
>
> ...except that it's now leaked for all error cases but the first.

Brain fart caused by looking at the RHEL code, will respin.

Re: [Qemu-devel] [PATCH 2/4] scsi-disk: qemu_vfree(NULL) is fine, simplify

[Paolo Bonzini]

Il 15/01/2013 14:23, Markus Armbruster ha scritto:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/scsi-disk.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index f8d7ef3..96db9a7 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -85,9 +85,7 @@ static void scsi_free_request(SCSIRequest *req)
>  {
>      SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>  
> -    if (r->iov.iov_base) {
> -        qemu_vfree(r->iov.iov_base);
> -    }
> +    qemu_vfree(r->iov.iov_base);
>  }
>  
>  /* Helper function for command completion with sense.  */
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 4/4] block: Fix how mirror_run() frees its buffer

[Paolo Bonzini]

Il 15/01/2013 15:20, Markus Armbruster ha scritto:
>> > Am 15.01.2013 14:23, schrieb Markus Armbruster:
>>> >> It allocates with qemu_blockalign(), therefore it must free with
>>> >> qemu_vfree(), not g_free().
>>> >> 
>>> >> Since I'm touching it anyway, move the free to a more obviosly correct
>>> >> place.
>> >
>> > ...except that it's now leaked for all error cases but the first.
> Brain fart caused by looking at the RHEL code, will respin.
> 
> 

I'll pick this in my own mirror series, if you don't mind.

Paolo

Re: [Qemu-devel] [PATCH 0/4] Fixes around qemu_vfree()

[Stefan Hajnoczi]

On Tue, Jan 15, 2013 at 02:23:36PM +0100, Markus Armbruster wrote:
> Markus Armbruster (4):
>   w32: Make qemu_vfree() accept NULL like the POSIX implementation
>   scsi-disk: qemu_vfree(NULL) is fine, simplify
>   win32-aio: Fix how win32_aio_process_completion() frees buffer
>   block: Fix how mirror_run() frees its buffer
> 
>  block/mirror.c     | 2 +-
>  block/win32-aio.c  | 2 +-
>  hw/scsi-disk.c     | 4 +---
>  util/oslib-win32.c | 4 +++-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> -- 
> 1.7.11.7
> 

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

Stefan