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

[Joel Schopp]

Thanks for the feedback.

On 02/27/2013 02:28 AM, Andreas Färber wrote:
> Am 27.02.2013 00:03, schrieb address@hidden:
> > These patches implement asn1 ber visitors for encoding and decoding data.
>
> Would be good to not be lazy and spell them correctly in at least one of
> the two lines of the commit where they're being introduced.

I will change "asn1 ber" to "ASN.1 BER"

> I would suggest to place your headers into existing include/qapi/ for
> instance and the remaining source files into qapi/ or a subdirectory
> thereof rather than a new top-level directory.
>
> I also note that you're not changing MAINTAINERS, so the new files don't
> have a maintainer assigned that gets CC'ed on changes.

I'll update MAINTAINERS and move the headers into include.

As for the location of the source files I think where the patch puts 
them now makes most sense, I don't think qapi makes sense.  But the 
location isn't important to me so I'll put them wherever.  Anybody else 
have an opinion on the best location?

> > +ber-nested-y = ber-input-visitor.o ber-output-visitor.o ber-common.o
> > +ber-obj-y = $(addprefix ber/, $(ber-nested-y))
> > +
> > +common-obj-y += qemu-char.o qemu-file.o $(ber-obj-y) #aio.o
>
> I don't see any justification for that statement? You should be able to
> add whatever file you like on a separate Makefile line without problems.
>
> Also I believe you are breaking dependency handling by not just doing
> common-obj-y += ber/
> utilising our existing object file unnesting rules.

That's what I thought as well, but it didn't compile.  This does.

> >   common-obj-y += block-migration.o
> >   common-obj-y += page_cache.o xbzrle.o
> >
> > @@ -116,4 +121,7 @@ nested-vars += \
> >         qga-obj-y \
> >         block-obj-y \
> >         common-obj-y
> > +# \
> > +#      ber-obj-y
> > +
>
> > Index: b/Makefile.objs
> > ===================================================================
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -57,7 +57,12 @@ common-obj-$(CONFIG_POSIX) += os-posix.o
> >   common-obj-$(CONFIG_LINUX) += fsdev/
> >
> >   common-obj-y += migration.o migration-tcp.o
> > -common-obj-y += qemu-char.o qemu-file.o #aio.o
> > +
> > +#ber has to be linked against qemu-file.o so we must add them on the same line
>
> Commented-out code.

Will remove

> >   dummy := $(call unnest-vars)
> > Index: b/ber/ber-common.c
> [snip]
>
> I wrote an ASN.1 implementation once, and it seems to me you are mixing
> up ASN.1 and BER here in your enum/array/... naming. The types are
> general ASN.1 concepts and apply to other encodings as well, no?

When/if we implement other ASN.1 encodings we can revisit what to name 
them enums/array/structs, but for now it highly increases readability if 
they are named after what they are used for.

> Also I remember that BER has disjoint CER and DER flavors that I don't
> see your output visitor distinguishing on brief sight, only P/C.
> Input visitor ideally handles both fine.

I'd refer you to the comment on ber/ber.h:
+/* This is a subset of BER for QEMU use. */
+/* QEMU will use the DER encoding always with one extension from
+ * CER: SET and SEQUENCE types can have indefinite-length encoding
+ * if the encoding is not all immediately available.
+ *
+ * We assume that SET encodings can be available or not available,
+ * and that SEQUENCE encodings are available unless a SEQUENCE includes
+ * a non-available SET.
+ *
+ * The last is an extension to allow an arbitrarily large SET
+ * to be produced online without knowing the length in advance.
+ *
+ * All types used shall be universal, with explicit tagging, to simplify
+ * use by external tools.