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

[Andreas Färber]

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.

> 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 +
>  ber/ber.h                   |   85 +++
>  include/qapi/qmp/qerror.h   |    6 
>  include/qapi/visitor-impl.h |   23 +
>  include/qapi/visitor.h      |   10 
>  qapi/qapi-visit-core.c      |   30 +
>  12 files changed, 1811 insertions(+), 1 deletion(-)

So now we're moving from crowded directories to every IBM contributor
creating their own personal subdirectory... (cf. TPM)

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.

> 
> 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
> +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.

>  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
> +

Commented-out code.

>  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?

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.

ASN.1
- textual
- BER
  - CER
  - DER
- PER (?)
- XER

While XER being XML-based just like the textual ASN.1 notation is
everything but efficient for transmission, keeping our options open to
implement either of these later for debugging purposes (e.g., verifying
VMState format) would be nice.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg