Am 27.02.2013 00:03, schrieb [email protected]: > 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: <[email protected]> > Content-Disposition: inline; filename=asn1_all.diff > > Signed-off-by: Stefan Berger <[email protected]> > Signed-off-by: Joel Schopp <[email protected]> > --- > 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
