Hey,
I've now pushed your patch along with the test case.
Christophe
On Fri, Jul 31, 2015 at 03:02:51PM +0200, Christophe Fergeau wrote:
> 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
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
pgpsFajVour78.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel