* [Qemu-devel] [PATCH 2/2] Tests: QEMUSizedBuffer/QEMUBuffer
2014-12-12 9:53 [Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated Yang Hongyang
@ 2014-12-12 9:53 ` Yang Hongyang
2014-12-16 13:37 ` Dr. David Alan Gilbert
2014-12-16 2:20 ` [Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated Hongyang Yang
2014-12-16 13:34 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 5+ messages in thread
From: Yang Hongyang @ 2014-12-12 9:53 UTC (permalink / raw)
To: qemu-devel; +Cc: Yang Hongyang, Dr. David Alan Gilbert, Juan Quintela
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH 2/2] Tests: QEMUSizedBuffer/QEMUBuffer
2014-12-12 9:53 ` [Qemu-devel] [PATCH 2/2] Tests: QEMUSizedBuffer/QEMUBuffer Yang Hongyang
@ 2014-12-16 13:37 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 5+ messages in thread
From: Dr. David Alan Gilbert @ 2014-12-16 13:37 UTC (permalink / raw)
To: Yang Hongyang; +Cc: amit.shah, qemu-devel, Juan Quintela
* 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated
2014-12-12 9:53 [Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated Yang Hongyang
2014-12-12 9:53 ` [Qemu-devel] [PATCH 2/2] Tests: QEMUSizedBuffer/QEMUBuffer Yang Hongyang
@ 2014-12-16 2:20 ` Hongyang Yang
2014-12-16 13:34 ` Dr. David Alan Gilbert
2 siblings, 0 replies; 5+ messages in thread
From: Hongyang Yang @ 2014-12-16 2:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Dr. David Alan Gilbert, Juan Quintela
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated
2014-12-12 9:53 [Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated Yang Hongyang
2014-12-12 9:53 ` [Qemu-devel] [PATCH 2/2] Tests: QEMUSizedBuffer/QEMUBuffer Yang Hongyang
2014-12-16 2:20 ` [Qemu-devel] [PATCH 1/2] QEMUSizedBuffer: only free qsb that qemu_bufopen allocated Hongyang Yang
@ 2014-12-16 13:34 ` Dr. David Alan Gilbert
2 siblings, 0 replies; 5+ messages in thread
From: Dr. David Alan Gilbert @ 2014-12-16 13:34 UTC (permalink / raw)
To: Yang Hongyang; +Cc: amit.shah, qemu-devel, Juan Quintela
* 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
^ permalink raw reply [flat|nested] 5+ messages in thread