[PATCH 2/3][RfC] Add utilities for encoding BTLVs and CTLVs.

[PATCH 2/3][RfC] Add utilities for encoding BTLVs and CTLVs.

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 13552 bytes --]

---
 src/simutil.c |  396 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/simutil.h |   44 +++++++
 2 files changed, 440 insertions(+), 0 deletions(-)

diff --git a/src/simutil.c b/src/simutil.c
index 822938c..ab2e421 100644
--- a/src/simutil.c
+++ b/src/simutil.c
@@ -486,6 +486,402 @@ static const guint8 *ber_tlv_find_by_tag(const guint8 *pdu, guint8 in_tag,
 	return NULL;
 }
 
+gboolean ber_tlv_constr_init(struct ber_tlv_constr *constr,
+				unsigned char *pdu, unsigned int size)
+{
+	if (size < 2)
+		return FALSE;
+
+	constr->pdu = pdu;
+	constr->pos = 0;
+	constr->max = size;
+	constr->parent = NULL;
+
+	return TRUE;
+}
+
+static inline unsigned int ber_tlv_get_tag_len(unsigned char *start)
+{
+	int len = 1;
+
+	if (bit_field(start[0], 0, 4) == 0x1f)
+		while (start[len] & 0x80)
+			len++;
+
+	return len;
+}
+
+static inline unsigned int ber_tlv_get_len_len(unsigned char *start)
+{
+	return *start >= 0x80 ? *start - 0x7f : 1;
+}
+
+gboolean ber_tlv_constr_next(struct ber_tlv_constr *constr)
+{
+	unsigned int taglen = ber_tlv_get_tag_len(constr->pdu + constr->pos);
+	unsigned int lenlen = 1;
+	unsigned int len = constr->pdu[constr->pos + taglen];
+
+	if (len >= 0x80) {
+		unsigned int extended_bytes = len - 0x80;
+		unsigned int i;
+
+		for (len = 0, i = 1; i <= extended_bytes; i++)
+			len = (len << 8) |
+				constr->pdu[constr->pos + taglen + i];
+
+		lenlen += extended_bytes;
+	}
+
+	if (constr->pos + taglen + lenlen + len + 2 > constr->max)
+		return FALSE;
+
+	constr->pos += taglen + lenlen + len;
+	constr->pdu[constr->pos + 0] = 0; /* Tag */
+	constr->pdu[constr->pos + 1] = 0; /* Length */
+
+	return TRUE;
+}
+
+gboolean ber_tlv_constr_set_tag(struct ber_tlv_constr *constr,
+				enum ber_tlv_data_type class,
+				enum ber_tlv_data_encoding_type encoding,
+				unsigned short tag)
+{
+	if (tag < 0x1f)
+		constr->pdu[constr->pos] =
+			(class << 6) | (encoding << 5) | tag;
+	else {
+		int taglen = 0;
+		int i;
+
+		while (tag >> (taglen * 7))
+			taglen += 1;
+
+		if (constr->pos + taglen + 2 > constr->max)
+			return FALSE;
+
+		constr->pdu[constr->pos] =
+			(class << 6) | (encoding << 5) | 0x1f;
+
+		for (i = 1; i < taglen; i += 1)
+			constr->pdu[constr->pos + i] = 0x80 |
+				(tag >> ((taglen - i)) * 7);
+
+		constr->pdu[constr->pos + i++] = tag & 0x7f;
+		constr->pdu[constr->pos + i] = 0; /* Length */
+	}
+
+	return TRUE;
+}
+
+/* Resize the TLV because the content of Value field needs more space.  If
+ * this TLV is part of another TLV, resize that one too.  */
+static unsigned char *ber_tlv_constr_update_len(struct ber_tlv_constr *constr,
+						unsigned int new_len,
+						unsigned int pos)
+{
+	unsigned char *tlv = constr->pdu + pos;
+	unsigned int taglen = ber_tlv_get_tag_len(tlv);
+	unsigned int lenlen = 1, new_lenlen = 1;
+	unsigned int len = tlv[taglen];
+	unsigned int tlv_len, new_tlv_len;
+	unsigned int added_len = 0;
+
+	/* How much space do we occupy now */
+	if (len >= 0x80) {
+		unsigned int extended_bytes = len - 0x80;
+		unsigned int i;
+
+		for (len = 0, i = 1; i <= extended_bytes; i++)
+			len = (len << 8) |
+				tlv[taglen + i];
+
+		lenlen += extended_bytes;
+	}
+
+	tlv_len = taglen + lenlen + len;
+
+	/* How much do we need */
+	if (new_len >= 0x80) {
+		unsigned int extended_bytes = 0;
+		while (new_len >> (extended_bytes * 8))
+			extended_bytes += 1;
+		new_lenlen += extended_bytes;
+	}
+
+	new_tlv_len = taglen + new_lenlen + new_len;
+
+	/* Check there is enough space */
+	if (constr->pos > pos)
+		added_len = constr->pos - (pos + tlv_len);
+
+	if (pos + new_tlv_len + added_len > constr->max)
+		return NULL;
+
+	if (constr->parent) {
+		unsigned char *new_pdu = ber_tlv_constr_update_len(
+						constr->parent,
+						pos + new_tlv_len + added_len,
+						constr->parent_pos);
+		if (new_pdu == NULL)
+			return NULL;
+
+		constr->pdu = new_pdu;
+		tlv = constr->pdu + pos;
+	}
+
+	if (added_len > 0 && new_tlv_len != tlv_len) {
+		memmove(tlv + new_tlv_len, tlv + tlv_len, added_len);
+		constr->pos += new_tlv_len - tlv_len;
+	}
+
+	if (new_lenlen != lenlen)
+		memmove(tlv + taglen + new_lenlen, tlv + taglen + lenlen,
+				tlv_len);
+
+	/* Write new length */
+	if (new_len < 0x80)
+		tlv[taglen] = new_len;
+	else {
+		unsigned int extended_bytes = 0;
+		unsigned int i;
+		while (new_len >> (extended_bytes * 8))
+			extended_bytes += 1;
+
+		for (i = 1; i <= extended_bytes; i++)
+			tlv[taglen + i] =
+				(new_len >> ((extended_bytes - i) * 8)) & 0xff;
+
+		tlv[taglen] = 0x80 + extended_bytes;
+	}
+
+	return tlv + taglen + new_lenlen;
+}
+
+gboolean ber_tlv_constr_set_length(struct ber_tlv_constr *constr,
+					unsigned int new_len)
+{
+	return ber_tlv_constr_update_len(constr, new_len, constr->pos) != NULL;
+}
+
+unsigned char *ber_tlv_constr_get_data(struct ber_tlv_constr *constr)
+{
+	unsigned char *tlv = constr->pdu + constr->pos;
+	unsigned int taglen = ber_tlv_get_tag_len(tlv);
+	unsigned int lenlen = ber_tlv_get_len_len(tlv + taglen);
+
+	return tlv + taglen + lenlen;
+}
+
+gboolean ber_tlv_constr_recurse(struct ber_tlv_constr *constr,
+				struct ber_tlv_constr *recurse)
+{
+	unsigned char *end = constr->pdu + constr->max;
+	unsigned char *data = ber_tlv_constr_get_data(constr);
+	gboolean r = ber_tlv_constr_init(recurse, data, end - data);
+
+	if (!r)
+		return FALSE;
+
+	recurse->parent = constr;
+	recurse->parent_pos = constr->pos;
+
+	return TRUE;
+}
+
+gboolean ber_tlv_constr_recurse_comprehension(struct ber_tlv_constr *constr,
+				struct comprehension_tlv_constr *recurse)
+{
+	unsigned char *end = constr->pdu + constr->max;
+	unsigned char *data = ber_tlv_constr_get_data(constr);
+	gboolean r = comprehension_tlv_constr_init(recurse, data, end - data);
+
+	if (!r)
+		return FALSE;
+
+	recurse->parent = constr;
+	recurse->parent_pos = constr->pos;
+
+	return TRUE;
+}
+
+gboolean comprehension_tlv_constr_init(struct comprehension_tlv_constr *constr,
+					unsigned char *pdu,
+					unsigned int size)
+{
+	if (size < 2)
+		return FALSE;
+
+	constr->pdu = pdu;
+	constr->pos = 0;
+	constr->max = size;
+	constr->parent = NULL;
+
+	return TRUE;
+}
+
+static inline unsigned int comprehension_tlv_get_tag_len(unsigned char *start)
+{
+	return bit_field(*start, 0, 7) == 0x7f ? 3 : 1;
+}
+
+static inline unsigned int comprehension_tlv_get_len_len(unsigned char *start)
+{
+	return *start >= 0x80 ? *start - 0x7f : 1;
+}
+
+gboolean comprehension_tlv_constr_next(struct comprehension_tlv_constr *constr)
+{
+	unsigned int taglen =
+		comprehension_tlv_get_tag_len(constr->pdu + constr->pos);
+	unsigned int lenlen = 1;
+	unsigned int len = constr->pdu[constr->pos + taglen];
+
+	if (len >= 0x80) {
+		unsigned int extended_bytes = len - 0x80;
+		unsigned int i;
+
+		for (len = 0, i = 1; i <= extended_bytes; i++)
+			len = (len << 8) |
+				constr->pdu[constr->pos + taglen + i];
+
+		lenlen += extended_bytes;
+	}
+
+	if (constr->pos + taglen + lenlen + len + 2 > constr->max)
+		return FALSE;
+
+	constr->pos += taglen + lenlen + len;
+	constr->pdu[constr->pos + 0] = 0; /* Tag */
+	constr->pdu[constr->pos + 1] = 0; /* Length */
+
+	return TRUE;
+}
+
+gboolean comprehension_tlv_constr_set_tag(
+					struct comprehension_tlv_constr *constr,
+					gboolean cr, unsigned short tag)
+{
+	if (tag < 0x7f)
+		constr->pdu[constr->pos] = (cr ? 0x80 : 0x00) | tag;
+	else {
+		if (constr->pos + 4 > constr->max)
+			return FALSE;
+
+		constr->pdu[constr->pos] = 0x7f;
+		constr->pdu[constr->pos + 1] = (cr ? 0x80 : 0x00) | (tag >> 8);
+		constr->pdu[constr->pos + 2] = tag & 0xff;
+		constr->pdu[constr->pos + 3] = 0;
+	}
+
+	return TRUE;
+}
+
+/* Resize the TLV because the content of Value field needs more space.  If
+ * this TLV is part of another TLV, resize that one too.  */
+static unsigned char *comprehension_tlv_constr_update_len(
+					struct comprehension_tlv_constr *constr,
+					unsigned int new_len,
+					unsigned int pos)
+{
+	unsigned char *tlv = constr->pdu + pos;
+	unsigned int taglen = comprehension_tlv_get_tag_len(tlv);
+	unsigned int lenlen = 1, new_lenlen = 1;
+	unsigned int len = tlv[taglen];
+	unsigned int ctlv_len, new_ctlv_len;
+	unsigned int added_len = 0;
+
+	/* How much space do we occupy now */
+	if (len >= 0x80) {
+		unsigned int extended_bytes = len - 0x80;
+		unsigned int i;
+
+		for (len = 0, i = 1; i <= extended_bytes; i++)
+			len = (len << 8) |
+				tlv[taglen + i];
+
+		lenlen += extended_bytes;
+	}
+
+	ctlv_len = taglen + lenlen + len;
+
+	/* How much do we need */
+	if (new_len >= 0x80) {
+		unsigned int extended_bytes = 0;
+		while (new_len >> (extended_bytes * 8))
+			extended_bytes += 1;
+		new_lenlen += extended_bytes;
+	}
+
+	new_ctlv_len = taglen + new_lenlen + new_len;
+
+	/* Check there is enough space */
+	if (constr->pos > pos)
+		added_len = constr->pos - (pos + ctlv_len);
+
+	if (pos + new_ctlv_len + added_len > constr->max)
+		return NULL;
+
+	if (constr->parent) {
+		unsigned char *new_pdu = ber_tlv_constr_update_len(
+						constr->parent,
+						pos + new_ctlv_len + added_len,
+						constr->parent_pos);
+		if (new_pdu == NULL)
+			return NULL;
+
+		constr->pdu = new_pdu;
+		tlv = constr->pdu + pos;
+	}
+
+	if (added_len > 0) {
+		memmove(tlv + new_ctlv_len, tlv + ctlv_len, added_len);
+		constr->pos += new_ctlv_len - ctlv_len;
+	}
+
+	if (new_lenlen > lenlen)
+		memmove(tlv + taglen + new_lenlen, tlv + taglen + lenlen,
+				new_lenlen - lenlen);
+
+	/* Write new length */
+	if (new_len < 0x80)
+		tlv[taglen] = new_len;
+	else {
+		unsigned int extended_bytes = 0;
+		unsigned int i;
+		while (new_len >> (extended_bytes * 8))
+			extended_bytes += 1;
+
+		for (i = 1; i <= extended_bytes; i++)
+			tlv[taglen + i] =
+				(new_len >> ((extended_bytes - i) * 8)) & 0xff;
+
+		tlv[taglen] = 0x80 + extended_bytes;
+	}
+
+	return tlv + taglen + new_lenlen;
+}
+
+gboolean comprehension_tlv_constr_set_length(
+					struct comprehension_tlv_constr *constr,
+					unsigned int new_len)
+{
+	return comprehension_tlv_constr_update_len(constr,
+							new_len,
+							constr->pos) != NULL;
+}
+
+unsigned char *comprehension_tlv_constr_get_data(
+				struct comprehension_tlv_constr *constr)
+{
+	unsigned char *tlv = constr->pdu + constr->pos;
+	unsigned int taglen = comprehension_tlv_get_tag_len(tlv);
+	unsigned int lenlen = comprehension_tlv_get_len_len(tlv + taglen);
+
+	return tlv + taglen + lenlen;
+}
+
 static char *sim_network_name_parse(const unsigned char *buffer, int length,
 					gboolean *add_ci)
 {
diff --git a/src/simutil.h b/src/simutil.h
index 7590cca..60db647 100644
--- a/src/simutil.h
+++ b/src/simutil.h
@@ -114,6 +114,22 @@ struct ber_tlv_iter {
 	const unsigned char *data;
 };
 
+struct ber_tlv_constr {
+	unsigned int max;
+	unsigned int pos;
+	unsigned char *pdu;
+	struct ber_tlv_constr *parent;
+	unsigned int parent_pos;
+};
+
+struct comprehension_tlv_constr {
+	unsigned int max;
+	unsigned int pos;
+	unsigned char *pdu;
+	struct ber_tlv_constr *parent;
+	unsigned int parent_pos;
+};
+
 void simple_tlv_iter_init(struct simple_tlv_iter *iter,
 				const unsigned char *pdu, unsigned int len);
 gboolean simple_tlv_iter_next(struct simple_tlv_iter *iter);
@@ -132,6 +148,18 @@ unsigned int comprehension_tlv_iter_get_length(
 const unsigned char *comprehension_tlv_iter_get_data(
 					struct comprehension_tlv_iter *iter);
 
+gboolean comprehension_tlv_constr_init(struct comprehension_tlv_constr *constr,
+					unsigned char *pdu, unsigned int size);
+gboolean comprehension_tlv_constr_next(struct comprehension_tlv_constr *constr);
+gboolean comprehension_tlv_constr_set_tag(
+					struct comprehension_tlv_constr *constr,
+					gboolean cr, unsigned short tag);
+gboolean comprehension_tlv_constr_set_length(
+					struct comprehension_tlv_constr *constr,
+					unsigned int len);
+unsigned char *comprehension_tlv_constr_get_data(
+				struct comprehension_tlv_constr *constr);
+
 void ber_tlv_iter_init(struct ber_tlv_iter *iter, const unsigned char *pdu,
 			unsigned int len);
 /*
@@ -164,6 +192,22 @@ void ber_tlv_iter_recurse_simple(struct ber_tlv_iter *iter,
 void ber_tlv_iter_recurse_comprehension(struct ber_tlv_iter *iter,
 					struct comprehension_tlv_iter *recurse);
 
+gboolean ber_tlv_constr_init(struct ber_tlv_constr *constr,
+				unsigned char *pdu, unsigned int size);
+gboolean ber_tlv_constr_set_tag(struct ber_tlv_constr *constr,
+				enum ber_tlv_data_type class,
+				enum ber_tlv_data_encoding_type encoding,
+				unsigned short tag);
+gboolean ber_tlv_constr_set_length(struct ber_tlv_constr *constr,
+					unsigned int len);
+unsigned char *ber_tlv_constr_get_data(struct ber_tlv_constr *constr);
+gboolean ber_tlv_constr_next(struct ber_tlv_constr *constr);
+gboolean ber_tlv_constr_recurse(struct ber_tlv_constr *constr,
+				struct ber_tlv_constr *recurse);
+gboolean ber_tlv_constr_recurse_comprehension(struct ber_tlv_constr *constr,
+				struct comprehension_tlv_constr *recurse);
+
+
 struct sim_eons *sim_eons_new(int pnn_records);
 void sim_eons_add_pnn_record(struct sim_eons *eons, int record,
 				const guint8 *tlv, int length);
-- 
1.6.1

Re: [PATCH 2/3][RfC] Add utilities for encoding BTLVs and CTLVs.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 6673 bytes --]

Hi Andrew,

> +gboolean ber_tlv_constr_init(struct ber_tlv_constr *constr,
> +				unsigned char *pdu, unsigned int size)
> +{
> +	if (size < 2)
> +		return FALSE;
> +
> +	constr->pdu = pdu;
> +	constr->pos = 0;
> +	constr->max = size;
> +	constr->parent = NULL;
> +
> +	return TRUE;
> +}

So I think for the case of the encoder we should bite the bullet and use 
something like a g_byte_array.  It will be really hard for us to guess the 
correct buffer size.

> +/* Resize the TLV because the content of Value field needs more space.  If
> + * this TLV is part of another TLV, resize that one too.  */
> +static unsigned char *ber_tlv_constr_update_len(struct ber_tlv_constr
>  *constr, +						unsigned int new_len,
> +						unsigned int pos)
> +{
> +	unsigned char *tlv = constr->pdu + pos;
> +	unsigned int taglen = ber_tlv_get_tag_len(tlv);
> +	unsigned int lenlen = 1, new_lenlen = 1;
> +	unsigned int len = tlv[taglen];
> +	unsigned int tlv_len, new_tlv_len;
> +	unsigned int added_len = 0;
> +
> +	/* How much space do we occupy now */
> +	if (len >= 0x80) {
> +		unsigned int extended_bytes = len - 0x80;
> +		unsigned int i;
> +
> +		for (len = 0, i = 1; i <= extended_bytes; i++)
> +			len = (len << 8) |
> +				tlv[taglen + i];
> +
> +		lenlen += extended_bytes;
> +	}
> +
> +	tlv_len = taglen + lenlen + len;
> +
> +	/* How much do we need */
> +	if (new_len >= 0x80) {
> +		unsigned int extended_bytes = 0;
> +		while (new_len >> (extended_bytes * 8))
> +			extended_bytes += 1;
> +		new_lenlen += extended_bytes;
> +	}
> +
> +	new_tlv_len = taglen + new_lenlen + new_len;
> +
> +	/* Check there is enough space */
> +	if (constr->pos > pos)
> +		added_len = constr->pos - (pos + tlv_len);
> +
> +	if (pos + new_tlv_len + added_len > constr->max)
> +		return NULL;
> +
> +	if (constr->parent) {
> +		unsigned char *new_pdu = ber_tlv_constr_update_len(
> +						constr->parent,
> +						pos + new_tlv_len + added_len,
> +						constr->parent_pos);
> +		if (new_pdu == NULL)
> +			return NULL;
> +
> +		constr->pdu = new_pdu;
> +		tlv = constr->pdu + pos;
> +	}
> +
> +	if (added_len > 0 && new_tlv_len != tlv_len) {
> +		memmove(tlv + new_tlv_len, tlv + tlv_len, added_len);
> +		constr->pos += new_tlv_len - tlv_len;
> +	}
> +
> +	if (new_lenlen != lenlen)
> +		memmove(tlv + taglen + new_lenlen, tlv + taglen + lenlen,
> +				tlv_len);
> +
> +	/* Write new length */
> +	if (new_len < 0x80)
> +		tlv[taglen] = new_len;
> +	else {
> +		unsigned int extended_bytes = 0;
> +		unsigned int i;
> +		while (new_len >> (extended_bytes * 8))
> +			extended_bytes += 1;
> +
> +		for (i = 1; i <= extended_bytes; i++)
> +			tlv[taglen + i] =
> +				(new_len >> ((extended_bytes - i) * 8)) & 0xff;
> +
> +		tlv[taglen] = 0x80 + extended_bytes;
> +	}
> +
> +	return tlv + taglen + new_lenlen;
> +}

Let me propose something, tell me if this can be made to work potentially more 
efficiently:

- When we init a ber_tlv, reserve a header at the beginning of the buffer to 
the max size of the tag + length.  The data with CTLVs will be written after 
that header.
- When we allocate a CTLV, we can set a hint whether this is a potentially 
'relocatable' CTLV, or this CTLV will always be of size less than 127 bytes.
- When we're done adding CTLVs, we can group our 'memmoves' of those CTLVs 
which are not relocatable.  For relocatable CTLVs we can set an initial flag on 
the tag from the set of reserved tags, e.g. 00, FF, etc.
- In case the CTLV reaches maximum size, it becomes non-relocatable.
- The relocation of BER-TLV is not necessary as we can fill the tag and length 
information at an offset from the beginning such that the data will follow 
immediately afterwards.  We simply return the data starting at this offset.
- This should hopefully result in not having to use memmove in the most common 
case.

I hope that made sense :)

> +gboolean comprehension_tlv_constr_init(struct comprehension_tlv_constr
>  *constr, +					unsigned char *pdu, unsigned int size);
> +gboolean comprehension_tlv_constr_next(struct comprehension_tlv_constr
>  *constr); +gboolean comprehension_tlv_constr_set_tag(
> +					struct comprehension_tlv_constr *constr,
> +					gboolean cr, unsigned short tag);
> +gboolean comprehension_tlv_constr_set_length(
> +					struct comprehension_tlv_constr *constr,
> +					unsigned int len);
> +unsigned char *comprehension_tlv_constr_get_data(
> +				struct comprehension_tlv_constr *constr);
> +
>  void ber_tlv_iter_init(struct ber_tlv_iter *iter, const unsigned char
>  *pdu, unsigned int len);
>  /*
> @@ -164,6 +192,22 @@ void ber_tlv_iter_recurse_simple(struct ber_tlv_iter
>  *iter, void ber_tlv_iter_recurse_comprehension(struct ber_tlv_iter *iter,
>  struct comprehension_tlv_iter *recurse);
> 
> +gboolean ber_tlv_constr_init(struct ber_tlv_constr *constr,
> +				unsigned char *pdu, unsigned int size);
> +gboolean ber_tlv_constr_set_tag(struct ber_tlv_constr *constr,
> +				enum ber_tlv_data_type class,
> +				enum ber_tlv_data_encoding_type encoding,
> +				unsigned short tag);
> +gboolean ber_tlv_constr_set_length(struct ber_tlv_constr *constr,
> +					unsigned int len);
> +unsigned char *ber_tlv_constr_get_data(struct ber_tlv_constr *constr);
> +gboolean ber_tlv_constr_next(struct ber_tlv_constr *constr);
> +gboolean ber_tlv_constr_recurse(struct ber_tlv_constr *constr,
> +				struct ber_tlv_constr *recurse);
> +gboolean ber_tlv_constr_recurse_comprehension(struct ber_tlv_constr
>  *constr, +				struct comprehension_tlv_constr *recurse);
> +
> +
>  struct sim_eons *sim_eons_new(int pnn_records);
>  void sim_eons_add_pnn_record(struct sim_eons *eons, int record,
>  				const guint8 *tlv, int length);
> 

I think we should not try to make generic ber/ctlv encoding utilities, lets 
concentrate on doing the sim toolkit ones only.  This will allow us to make 
several simplifications (e.g. not worry about multi-byte ber-tlv tags, etc)

Something like:

stk_ber_tlv_builder_init(iter, tag);
stk_ber_tlv_builder_open_container(iter, container, tag);
stk_ber_tlv_builder_close_container(iter, container);
stk_ber_tlv_builder_optimize(iter);

stk_ctlv_append_byte(iter, byte);
stk_ctlv_append_short(iter, short);
stk_ctlv_append_data(iter, data, len);
stk_ctlv_append_text(iter, text) -> If we can make this work, converts to dcs, 
data format pair

Since Terminal Responses are not wrapped in BER-TLVs, we can simply invent a 
dummy value and return the data at offset starting at the first CTLV.

Comments?

Regards,
-Denis

Re: [PATCH 2/3][RfC] Add utilities for encoding BTLVs and CTLVs.

[andrzej zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2844 bytes --]

Hi,

On 23 April 2010 22:09, Denis Kenzior <denkenz@gmail.com> wrote:
> So I think for the case of the encoder we should bite the bullet and use
> something like a g_byte_array.  It will be really hard for us to guess the
> correct buffer size.

Ok with me, although most of the time we have a limit of 256 bytes per
PDU or some other limit.  We could also use a growing static buffer to
avoid having to free the array.

>
> Let me propose something, tell me if this can be made to work potentially more
> efficiently:
>
> - When we init a ber_tlv, reserve a header at the beginning of the buffer to
> the max size of the tag + length.  The data with CTLVs will be written after
> that header.
> - When we allocate a CTLV, we can set a hint whether this is a potentially
> 'relocatable' CTLV, or this CTLV will always be of size less than 127 bytes.
> - When we're done adding CTLVs, we can group our 'memmoves' of those CTLVs
> which are not relocatable.  For relocatable CTLVs we can set an initial flag on
> the tag from the set of reserved tags, e.g. 00, FF, etc.
> - In case the CTLV reaches maximum size, it becomes non-relocatable.
> - The relocation of BER-TLV is not necessary as we can fill the tag and length
> information at an offset from the beginning such that the data will follow
> immediately afterwards.  We simply return the data starting at this offset.
> - This should hopefully result in not having to use memmove in the most common
> case.

I don't think the memmove is much of a problem.  In the most common
case there's no move needed, e.g. in none of the TERMINAL RESPONSEs
there will any.  There's one in the mms pdu test I sent.  There will
never be more than one per BER TLV, and it can be of only 127 bytes
worst case, so probably faster than all the memory allocs glib does if
we use GByteArray.

>
> I think we should not try to make generic ber/ctlv encoding utilities, lets
> concentrate on doing the sim toolkit ones only.  This will allow us to make
> several simplifications (e.g. not worry about multi-byte ber-tlv tags, etc)
>
> Something like:
>
> stk_ber_tlv_builder_init(iter, tag);
> stk_ber_tlv_builder_open_container(iter, container, tag);
> stk_ber_tlv_builder_close_container(iter, container);
> stk_ber_tlv_builder_optimize(iter);
>
> stk_ctlv_append_byte(iter, byte);
> stk_ctlv_append_short(iter, short);
> stk_ctlv_append_data(iter, data, len);
> stk_ctlv_append_text(iter, text) -> If we can make this work, converts to dcs,
> data format pair

Sounds ok, but in the end it's a slight complication because you have
to call "close_container" and "optimize" :).

Would stk_ber_tlv_builder_open_container always start a CTLV?
Do you prefer these calls use the generic calls, or do the encoding
entirely in stkutil.c?

Regards

Re: [PATCH 2/3][RfC] Add utilities for encoding BTLVs and CTLVs.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4036 bytes --]

Hi Andrew,

> Hi,
> 
> On 23 April 2010 22:09, Denis Kenzior <denkenz@gmail.com> wrote:
> > So I think for the case of the encoder we should bite the bullet and use
> > something like a g_byte_array.  It will be really hard for us to guess
> > the correct buffer size.
> 
> Ok with me, although most of the time we have a limit of 256 bytes per
> PDU or some other limit.  We could also use a growing static buffer to
> avoid having to free the array.

The biggest issue is the text string, you really can't predict the buffer for 
that one.  And I don't want to see 16K+ buffers pre-allocated for this stuff :)

The static buffer of 256 bytes is an optimization we can certainly make for the 
90% case.

> 
> > Let me propose something, tell me if this can be made to work potentially
> > more efficiently:
> >
> > - When we init a ber_tlv, reserve a header at the beginning of the buffer
> > to the max size of the tag + length.  The data with CTLVs will be written
> > after that header.
> > - When we allocate a CTLV, we can set a hint whether this is a
> > potentially 'relocatable' CTLV, or this CTLV will always be of size less
> > than 127 bytes. - When we're done adding CTLVs, we can group our
> > 'memmoves' of those CTLVs which are not relocatable.  For relocatable
> > CTLVs we can set an initial flag on the tag from the set of reserved
> > tags, e.g. 00, FF, etc.
> > - In case the CTLV reaches maximum size, it becomes non-relocatable.
> > - The relocation of BER-TLV is not necessary as we can fill the tag and
> > length information at an offset from the beginning such that the data
> > will follow immediately afterwards.  We simply return the data starting
> > at this offset. - This should hopefully result in not having to use
> > memmove in the most common case.
> 
> I don't think the memmove is much of a problem.  In the most common
> case there's no move needed, e.g. in none of the TERMINAL RESPONSEs
> there will any.  There's one in the mms pdu test I sent.  There will
> never be more than one per BER TLV, and it can be of only 127 bytes
> worst case, so probably faster than all the memory allocs glib does if
> we use GByteArray.

Do you mean C-TLV?  And why 127 bytes?  Wouldn't you have to memmove a 
potentially large array in case a large display text is added?

> 
> > I think we should not try to make generic ber/ctlv encoding utilities,
> > lets concentrate on doing the sim toolkit ones only.  This will allow us
> > to make several simplifications (e.g. not worry about multi-byte ber-tlv
> > tags, etc)
> >
> > Something like:
> >
> > stk_ber_tlv_builder_init(iter, tag);
> > stk_ber_tlv_builder_open_container(iter, container, tag);
> > stk_ber_tlv_builder_close_container(iter, container);
> > stk_ber_tlv_builder_optimize(iter);
> >
> > stk_ctlv_append_byte(iter, byte);
> > stk_ctlv_append_short(iter, short);
> > stk_ctlv_append_data(iter, data, len);
> > stk_ctlv_append_text(iter, text) -> If we can make this work, converts to
> > dcs, data format pair
> 
> Sounds ok, but in the end it's a slight complication because you have
> to call "close_container" and "optimize" :).

You can always do the magic to close the container automatically on an 
'open_container' and 'optimize' or re-optimize on each close_container similar 
to how you're doing it now.  In the end I mostly care about making this easy 
on the encoding code by not having to pre-allocate the buffers or composing the 
pdus before adding them to the TLV.

> 
> Would stk_ber_tlv_builder_open_container always start a CTLV?
> Do you prefer these calls use the generic calls, or do the encoding
> entirely in stkutil.c?

For the purposes of STK work, yes.  You can always come up with a better name 
:)  Right now I don't think we need to solve the generic ber-tlv / simple-tlv 
/ comprehension tlv composition problem, so doing it entirely in stkutil.c is 
fine.  Do you foresee a need to make these generic?

Regards,
-Denis