Thanks for the feedback.
On 02/27/2013 02:28 AM, Andreas Färber wrote:
Am 27.02.2013 00:03, schrieb jsch...@linux.vnet.ibm.com:
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.