[Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated

[Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated

[Yang Hongyang]

Only free qsb that qemu_bufopen allocated, and also allow
qemu_bufopen accept qsb as input for write operation. It
will make the API more logical:
1.If you create the QEMUSizedBuffer yourself, you need to
  free it by using qsb_free() but not depends on other API
  like qemu_fclose.
2.allow qemu_bufopen() accept QEMUSizedBuffer as input for
  write operation, otherwise, it will be a little strange
  for this API won't accept the second parameter.

This brings API change, since there are only 3
users of this API currently, this change only impact the
first one which will be fixed in patch 2 of this patchset,
so I think it is safe to do this change.

1     70  tests/test-vmstate.c <<open_mem_file_read>>
            return qemu_bufopen("r", qsb);
2    404  tests/test-vmstate.c <<test_save_noskip>>
            QEMUFile *fsave = qemu_bufopen("w", NULL);
3    424  tests/test-vmstate.c <<test_save_skip>>
            QEMUFile *fsave = qemu_bufopen("w", NULL);

Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
---
 qemu-file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/qemu-file.c b/qemu-file.c
index f938e36..52f8d69 100644
--- a/qemu-file.c
+++ b/qemu-file.c
@@ -904,6 +904,7 @@ QEMUSizedBuffer *qsb_clone(const QEMUSizedBuffer *qsb)
 typedef struct QEMUBuffer {
     QEMUSizedBuffer *qsb;
     QEMUFile *file;
+    bool qsb_allocated;
 } QEMUBuffer;
 
 static int buf_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
@@ -933,7 +934,9 @@ static int buf_close(void *opaque)
 {
     QEMUBuffer *s = opaque;
 
-    qsb_free(s->qsb);
+    if (s->qsb_allocated) {
+        qsb_free(s->qsb);
+    }
 
     g_free(s);
 
@@ -972,12 +975,11 @@ QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input)
     }
 
     s = g_malloc0(sizeof(QEMUBuffer));
-    if (mode[0] == 'r') {
-        s->qsb = input;
-    }
+    s->qsb = input;
 
     if (s->qsb == NULL) {
         s->qsb = qsb_create(NULL, 0);
+        s->qsb_allocated = true;
     }
     if (!s->qsb) {
         g_free(s);
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] Tests: QEMUSizedBuffer/QEMUBuffer

[Yang Hongyang]

Modify some of tests/test-vmstate.c due to qemu_bufopen() change.
If you create a QEMUSizedBuffer yourself, you have to explicitly
free it.

Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
---
 tests/test-vmstate.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index 5e0fd13..39b7b01 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -60,16 +60,6 @@ static QEMUFile *open_test_file(bool write)
     return qemu_fdopen(fd, write ? "wb" : "rb");
 }
 
-/* Open a read-only qemu-file from an existing memory block */
-static QEMUFile *open_mem_file_read(const void *data, size_t len)
-{
-    /* The qsb gets freed by qemu_fclose */
-    QEMUSizedBuffer *qsb = qsb_create(data, len);
-    g_assert(qsb);
-
-    return qemu_bufopen("r", qsb);
-}
-
 /*
  * Check that the contents of the memory-buffered file f match
  * the given size/data.
@@ -450,7 +440,9 @@ static void test_load_noskip(void)
         QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
     };
 
-    QEMUFile *loading = open_mem_file_read(buf, sizeof(buf));
+    QEMUSizedBuffer *qsb = qsb_create(buf, sizeof(buf));
+    g_assert(qsb);
+    QEMUFile *loading = qemu_bufopen("r", qsb);
     TestStruct obj = { .skip_c_e = false };
     vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
     g_assert(!qemu_file_get_error(loading));
@@ -461,6 +453,7 @@ static void test_load_noskip(void)
     g_assert_cmpint(obj.e, ==, 50);
     g_assert_cmpint(obj.f, ==, 60);
     qemu_fclose(loading);
+    qsb_free(qsb);
 }
 
 static void test_load_skip(void)
@@ -473,7 +466,9 @@ static void test_load_skip(void)
         QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
     };
 
-    QEMUFile *loading = open_mem_file_read(buf, sizeof(buf));
+    QEMUSizedBuffer *qsb = qsb_create(buf, sizeof(buf));
+    g_assert(qsb);
+    QEMUFile *loading = qemu_bufopen("r", qsb);
     TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
     vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
     g_assert(!qemu_file_get_error(loading));
@@ -484,6 +479,7 @@ static void test_load_skip(void)
     g_assert_cmpint(obj.e, ==, 500);
     g_assert_cmpint(obj.f, ==, 60);
     qemu_fclose(loading);
+    qsb_free(qsb);
 }
 
 int main(int argc, char **argv)
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated

[Hongyang Yang]

Ping!

在 12/12/2014 05:53 PM, Yang Hongyang 写道:
> Only free qsb that qemu_bufopen allocated, and also allow
> qemu_bufopen accept qsb as input for write operation. It
> will make the API more logical:
> 1.If you create the QEMUSizedBuffer yourself, you need to
>    free it by using qsb_free() but not depends on other API
>    like qemu_fclose.
> 2.allow qemu_bufopen() accept QEMUSizedBuffer as input for
>    write operation, otherwise, it will be a little strange
>    for this API won't accept the second parameter.
>
> This brings API change, since there are only 3
> users of this API currently, this change only impact the
> first one which will be fixed in patch 2 of this patchset,
> so I think it is safe to do this change.
>
> 1     70  tests/test-vmstate.c <<open_mem_file_read>>
>              return qemu_bufopen("r", qsb);
> 2    404  tests/test-vmstate.c <<test_save_noskip>>
>              QEMUFile *fsave = qemu_bufopen("w", NULL);
> 3    424  tests/test-vmstate.c <<test_save_skip>>
>              QEMUFile *fsave = qemu_bufopen("w", NULL);
>
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> ---
>   qemu-file.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-file.c b/qemu-file.c
> index f938e36..52f8d69 100644
> --- a/qemu-file.c
> +++ b/qemu-file.c
> @@ -904,6 +904,7 @@ QEMUSizedBuffer *qsb_clone(const QEMUSizedBuffer *qsb)
>   typedef struct QEMUBuffer {
>       QEMUSizedBuffer *qsb;
>       QEMUFile *file;
> +    bool qsb_allocated;
>   } QEMUBuffer;
>
>   static int buf_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
> @@ -933,7 +934,9 @@ static int buf_close(void *opaque)
>   {
>       QEMUBuffer *s = opaque;
>
> -    qsb_free(s->qsb);
> +    if (s->qsb_allocated) {
> +        qsb_free(s->qsb);
> +    }
>
>       g_free(s);
>
> @@ -972,12 +975,11 @@ QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input)
>       }
>
>       s = g_malloc0(sizeof(QEMUBuffer));
> -    if (mode[0] == 'r') {
> -        s->qsb = input;
> -    }
> +    s->qsb = input;
>
>       if (s->qsb == NULL) {
>           s->qsb = qsb_create(NULL, 0);
> +        s->qsb_allocated = true;
>       }
>       if (!s->qsb) {
>           g_free(s);
>

-- 
Thanks,
Yang.

Re: [Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated

[Dr. David Alan Gilbert]

* Yang Hongyang (yanghy@cn.fujitsu.com) wrote:
> Only free qsb that qemu_bufopen allocated, and also allow
> qemu_bufopen accept qsb as input for write operation. It
> will make the API more logical:
> 1.If you create the QEMUSizedBuffer yourself, you need to
>   free it by using qsb_free() but not depends on other API
>   like qemu_fclose.
> 2.allow qemu_bufopen() accept QEMUSizedBuffer as input for
>   write operation, otherwise, it will be a little strange
>   for this API won't accept the second parameter.
> 
> This brings API change, since there are only 3
> users of this API currently, this change only impact the
> first one which will be fixed in patch 2 of this patchset,
> so I think it is safe to do this change.
> 
> 1     70  tests/test-vmstate.c <<open_mem_file_read>>
>             return qemu_bufopen("r", qsb);
> 2    404  tests/test-vmstate.c <<test_save_noskip>>
>             QEMUFile *fsave = qemu_bufopen("w", NULL);
> 3    424  tests/test-vmstate.c <<test_save_skip>>
>             QEMUFile *fsave = qemu_bufopen("w", NULL);
> 
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>

Yes, this feels cleaner to me.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave


> ---
>  qemu-file.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-file.c b/qemu-file.c
> index f938e36..52f8d69 100644
> --- a/qemu-file.c
> +++ b/qemu-file.c
> @@ -904,6 +904,7 @@ QEMUSizedBuffer *qsb_clone(const QEMUSizedBuffer *qsb)
>  typedef struct QEMUBuffer {
>      QEMUSizedBuffer *qsb;
>      QEMUFile *file;
> +    bool qsb_allocated;
>  } QEMUBuffer;
>  
>  static int buf_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
> @@ -933,7 +934,9 @@ static int buf_close(void *opaque)
>  {
>      QEMUBuffer *s = opaque;
>  
> -    qsb_free(s->qsb);
> +    if (s->qsb_allocated) {
> +        qsb_free(s->qsb);
> +    }
>  
>      g_free(s);
>  
> @@ -972,12 +975,11 @@ QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input)
>      }
>  
>      s = g_malloc0(sizeof(QEMUBuffer));
> -    if (mode[0] == 'r') {
> -        s->qsb = input;
> -    }
> +    s->qsb = input;
>  
>      if (s->qsb == NULL) {
>          s->qsb = qsb_create(NULL, 0);
> +        s->qsb_allocated = true;
>      }
>      if (!s->qsb) {
>          g_free(s);
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] Tests: QEMUSizedBuffer/QEMUBuffer

[Dr. David Alan Gilbert]

* Yang Hongyang (yanghy@cn.fujitsu.com) wrote:
> Modify some of tests/test-vmstate.c due to qemu_bufopen() change.
> If you create a QEMUSizedBuffer yourself, you have to explicitly
> free it.
> 
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  tests/test-vmstate.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)

> 
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index 5e0fd13..39b7b01 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -60,16 +60,6 @@ static QEMUFile *open_test_file(bool write)
>      return qemu_fdopen(fd, write ? "wb" : "rb");
>  }
>  
> -/* Open a read-only qemu-file from an existing memory block */
> -static QEMUFile *open_mem_file_read(const void *data, size_t len)
> -{
> -    /* The qsb gets freed by qemu_fclose */
> -    QEMUSizedBuffer *qsb = qsb_create(data, len);
> -    g_assert(qsb);
> -
> -    return qemu_bufopen("r", qsb);
> -}
> -
>  /*
>   * Check that the contents of the memory-buffered file f match
>   * the given size/data.
> @@ -450,7 +440,9 @@ static void test_load_noskip(void)
>          QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
>      };
>  
> -    QEMUFile *loading = open_mem_file_read(buf, sizeof(buf));
> +    QEMUSizedBuffer *qsb = qsb_create(buf, sizeof(buf));
> +    g_assert(qsb);
> +    QEMUFile *loading = qemu_bufopen("r", qsb);
>      TestStruct obj = { .skip_c_e = false };
>      vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
>      g_assert(!qemu_file_get_error(loading));
> @@ -461,6 +453,7 @@ static void test_load_noskip(void)
>      g_assert_cmpint(obj.e, ==, 50);
>      g_assert_cmpint(obj.f, ==, 60);
>      qemu_fclose(loading);
> +    qsb_free(qsb);
>  }
>  
>  static void test_load_skip(void)
> @@ -473,7 +466,9 @@ static void test_load_skip(void)
>          QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
>      };
>  
> -    QEMUFile *loading = open_mem_file_read(buf, sizeof(buf));
> +    QEMUSizedBuffer *qsb = qsb_create(buf, sizeof(buf));
> +    g_assert(qsb);
> +    QEMUFile *loading = qemu_bufopen("r", qsb);
>      TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
>      vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
>      g_assert(!qemu_file_get_error(loading));
> @@ -484,6 +479,7 @@ static void test_load_skip(void)
>      g_assert_cmpint(obj.e, ==, 500);
>      g_assert_cmpint(obj.f, ==, 60);
>      qemu_fclose(loading);
> +    qsb_free(qsb);
>  }
>  
>  int main(int argc, char **argv)
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK