Re: [Qemu-devel] [PATCH 2/3] asn1 ber visitors

[mdroth]

On Wed, Feb 27, 2013 at 06:24:45PM -0500, Stefan Berger wrote:
> On 02/27/2013 05:57 PM, mdroth wrote:
> >On Tue, Feb 26, 2013 at 05:03:56PM -0600, address@hidden wrote:
> >>These patches implement asn1 ber visitors for encoding and decoding data.
> >>References: <address@hidden>
> >>Content-Disposition: inline; filename=asn1_all.diff
> >>
> >>Signed-off-by: Stefan Berger <address@hidden>
> >>Signed-off-by: Joel Schopp <address@hidden>
> >>---
> >>  Makefile.objs               |   10
> >>  ber/Makefile.objs           |    2
> >>  ber/ber-common.c            |   56 ++
> >>  ber/ber-input-visitor.c     |  969 
> >> ++++++++++++++++++++++++++++++++++++++++++++
> >>  ber/ber-input-visitor.h     |   30 +
> >>  ber/ber-output-visitor.c    |  563 +++++++++++++++++++++++++
> >>  ber/ber-output-visitor.h    |   28 +
> >Please break the input/output implementations out into separate patches,
> >and follow those up with test cases. See:
> >
> >tests/test-{string,qmp}-{input,output}-visitor.c
> >tests/test-visitor-serialization.c
> 
> I saw this with the other test cases... The problem with the ASN.1
> encoding is that writing ASN.1 into a buffer doesn't make much
> sense, unless of course you were to compare it against a binary
> string that was generated through other means, which I didn't do. So
> instead I wrote test cases that write the ASN.1 stream using the
> output visitor and then immediately feed the streams into the input
> visitor and then I compare the outcome against expected outcome,
> e.g., that a stucture's fields were properly filled. I hope this
> makes more sense in the ASN.1 case.

This is actually exactly what test-visitor-serialization.c does. It
feeds various normal/corner cases of visit_type_* into your output
visitor, takes that output and feeds it back into the input visitor,
then validates it against the original input. Just need to create
a SerializeOps implementation to drive your visitor and plug it
in like the others.

Just doing that would be a good start at least, but it would be really
nice to validate the encoding against some other reference implementation.
Have you looked into libsnacc? It seems to be the most readilly
available library. I wouldn't make it a formal build requirement, but it
would be nice to be able to execute test cases that use it if it's
present.

Even failing that, I personally wouldn't even mind just generating an
encoded blob outside the tree, and then checking that it along with the
textual description and steps to generate it, then validating the
visitors against it. It's not too far off from our "hand-written" JSON
to test the QMP visitors.

I don't think we have any reasonable assurance that our implementation
is correct otherwise.

> 
>    Stefan
> 
>