[Spice-devel] [PATCH spice-common RFC] Fix linearization of several marshallers with one item
Christophe Fergeau
cfergeau at redhat.com
Fri Jul 31 06:02:51 PDT 2015
On Fri, Jul 31, 2015 at 11:11:54AM +0200, Christophe Fergeau wrote:
> Hey,
>
> On Mon, Jul 27, 2015 at 01:27:14PM +0200, Javier Celaya wrote:
> > The linearization optimization that avoids copying only one item must
> > check that there are no further marshallers in the chain.
> > ---
> > common/marshaller.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/common/marshaller.c b/common/marshaller.c
> > index bd012d7..0c6680e 100644
> > --- a/common/marshaller.c
> > +++ b/common/marshaller.c
> > @@ -419,7 +419,7 @@ uint8_t *spice_marshaller_linearize(SpiceMarshaller *m, size_t skip_bytes,
> > /* Only supported for root marshaller */
> > assert(m->data->marshallers == m);
> >
> > - if (m->n_items == 1) {
> > + if (m->n_items == 1 && m->next == NULL) {
> > *free_res = FALSE;
> > if (m->items[0].len <= skip_bytes) {
> > *len = 0;
>
> Yes, this makes a lot of sense, though your follow-up explanation should
> be in the commit log. Thanks for tracking this down, ACK.
The attached patch exhibits the issue on unpatched spice-common (and
probably helped me understand a bit more your other patch with _flush()
in it ;)
Christophe
-------------- next part --------------
From f5f831afaf74e86256019b92437ab658eafe5a7e Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau at redhat.com>
Date: Fri, 31 Jul 2015 11:51:43 +0200
Subject: [spice-common] Add marshaller test case
This allows to test the spice_marshaller_linearize() fix which was sent
recently.
---
configure.ac | 1 +
tests/Makefile.am | 50 ++++++++++++++++++++++++++++++++++++++++++++
tests/test-marshallers.c | 39 ++++++++++++++++++++++++++++++++++
tests/test-marshallers.h | 11 ++++++++++
tests/test-marshallers.proto | 10 +++++++++
5 files changed, 111 insertions(+)
create mode 100644 tests/Makefile.am
create mode 100644 tests/test-marshallers.c
create mode 100644 tests/test-marshallers.h
create mode 100644 tests/test-marshallers.proto
diff --git a/configure.ac b/configure.ac
index 0bfc90c..acb91a6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -53,6 +53,7 @@ AC_CONFIG_FILES([
common/Makefile
python_modules/Makefile
codegen/Makefile
+ tests/Makefile
])
AH_BOTTOM([
diff --git a/tests/Makefile.am b/tests/Makefile.am
new file mode 100644
index 0000000..bb627f5
--- /dev/null
+++ b/tests/Makefile.am
@@ -0,0 +1,50 @@
+NULL =
+
+TESTS = test_marshallers
+check_PROGRAMS = test_marshallers
+test_marshallers_SOURCES = \
+ generated_test_marshallers.c \
+ generated_test_marshallers.h \
+ test-marshallers.c \
+ test-marshallers.h \
+ $(NULL)
+test_marshallers_CFLAGS = \
+ -I$(top_srcdir)/common \
+ $(GLIB2_CFLAGS) \
+ $(PROTOCOL_CFLAGS) \
+ $(NULL)
+test_marshallers_LDFLAGS = \
+ $(top_builddir)/common/libspice-common.la \
+ $(GLIB2_LIBS) \
+ $(NULL)
+
+# Avoid need for python(pyparsing) by end users
+TEST_MARSHALLERS = \
+ generated_test_marshallers.c \
+ generated_test_marshallers.h \
+ $(NULL)
+
+BUILT_SOURCES = $(TEST_MARSHALLERS)
+
+MARSHALLERS_DEPS = \
+ $(top_srcdir)/python_modules/__init__.py \
+ $(top_srcdir)/python_modules/codegen.py \
+ $(top_srcdir)/python_modules/demarshal.py \
+ $(top_srcdir)/python_modules/marshal.py \
+ $(top_srcdir)/python_modules/ptypes.py \
+ $(top_srcdir)/python_modules/spice_parser.py \
+ $(top_srcdir)/spice_codegen.py \
+ $(NULL)
+
+# Note despite being autogenerated these are not part of CLEANFILES, they are
+# actually a part of EXTRA_DIST, to avoid the need for pyparser by end users
+generated_test_marshallers.c: $(srcdir)/test-marshallers.proto $(MARSHALLERS_DEPS)
+ $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-marshallers --server --include test-marshallers.h $< $@ >/dev/null
+generated_test_marshallers.h: $(srcdir)/test-marshallers.proto $(MARSHALLERS_DEPS)
+ $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-marshallers --server --include test-marshallers.h -H $< $@ >/dev/null
+
+EXTRA_DIST = \
+ $(TEST_MARSHALLERS) \
+ $(NULL)
+
+-include $(top_srcdir)/git.mk
diff --git a/tests/test-marshallers.c b/tests/test-marshallers.c
new file mode 100644
index 0000000..e3e4d09
--- /dev/null
+++ b/tests/test-marshallers.c
@@ -0,0 +1,39 @@
+#include <glib.h>
+#include <string.h>
+#include <marshaller.h>
+
+#include <generated_test_marshallers.h>
+
+static uint8_t expected_data[] = { 0x02, 0x00, 0x00, 0x00, /* data_size */
+ 0x08, 0x00, 0x00, 0x00, /* data offset */
+ 0xef, 0xcd, 0xab, 0x90, 0x78, 0x56, 0x34, 0x12, /* data */
+ 0xef, 0xcd, 0xab, 0x90, 0x78, 0x56, 0x34, 0x12, /* data */
+};
+
+int main(int argc, char **argv)
+{
+ SpiceMarshaller *marshaller;
+ SpiceMsgMainShortDataSubMarshall *msg;
+ size_t len;
+ int free_res;
+ uint8_t *data;
+
+ msg = spice_malloc0(sizeof(SpiceMsgMainShortDataSubMarshall) + 2 * sizeof(uint64_t));
+ msg->data_size = 2;
+ msg->data[0] = 0x1234567890abcdef;
+ msg->data[1] = 0x1234567890abcdef;
+
+ marshaller = spice_marshaller_new();
+ spice_marshall_msg_main_ShortDataSubMarshall(marshaller, msg);
+ spice_marshaller_flush(marshaller);
+ data = spice_marshaller_linearize(marshaller, 0, &len, &free_res);
+ g_assert_cmpint(len, ==, G_N_ELEMENTS(expected_data));
+ g_assert_true(memcmp(data, expected_data, len) == 0);
+ if (free_res) {
+ free(data);
+ }
+ spice_marshaller_destroy(marshaller);
+ free(msg);
+
+ return 0;
+}
diff --git a/tests/test-marshallers.h b/tests/test-marshallers.h
new file mode 100644
index 0000000..9cd34c7
--- /dev/null
+++ b/tests/test-marshallers.h
@@ -0,0 +1,11 @@
+#include <stdint.h>
+
+#ifndef _H_TEST_MARSHALLERS
+
+typedef struct {
+ uint32_t data_size;
+ uint64_t data[];
+} SpiceMsgMainShortDataSubMarshall;
+
+#endif /* _H_TEST_MARSHALLERS */
+
diff --git a/tests/test-marshallers.proto b/tests/test-marshallers.proto
new file mode 100644
index 0000000..e360b09
--- /dev/null
+++ b/tests/test-marshallers.proto
@@ -0,0 +1,10 @@
+channel TestChannel {
+ message {
+ uint32 data_size;
+ uint64 *data[data_size] @marshall;
+ } ShortDataSubMarshall;
+};
+
+protocol Spice {
+ TestChannel main = 1;
+};
--
2.4.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150731/9f423691/attachment.sig>
More information about the Spice-devel
mailing list