Re: [PATCH spice-common RFC] Fix linearization of several marshallers with one item

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
From f5f831afaf74e86256019b92437ab658eafe5a7e Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
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

Attachment: pgpT33BhSqHpI.pgp
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]