On Mon, Aug 12, 2013 at 02:08:17PM +0200, Jacob Erlbeck wrote:
The 7bit<->8bit encoding/decoding functions
didn't check whether
there is still enough space in the destination buffer. Therefore a
buffer size parameter ('n' like in snprintf) has been added to each
of the functions which leads to truncation if the the buffer is too
small.
I think there has to be a comma and 'the the'
In addition, the return value of the decoding
functions has been
changed to number of characters written (excluding \0), so this
value is always equal to strlen(decoded) if n>0.
I commented that in the code, but maybe here as well. Maybe it is
easier to forbid n==0 and add an OSMO_ASSERT? The argument for that
is one can unconditionally printf the result?
+/*
+ * The following functions write at most n octets to the destination buffer
+ * ('decoded' or 'result') and return the number of octets written
(always
+ * <= n).
+ */
Doxygen has the concept of groups[1], do you want to use that for these
methods or do you just want to remove this comment?
msgb_put_u8(msg, ASN1_OCTET_STRING_TAG);
ussd_len_ptr = msgb_put(msg, 1);
data = msgb_put(msg, 0);
- gsm_7bit_encode_ussd(data, text, &len);
+ gsm_7bit_encode_n_ussd(data, msgb_tailroom(msg), text, &len);
i think there is no test coverage for all of these calls.
+ gsm_7bit_decode_n_ussd(req->text, sizeof
(req->text),
no space. :)
or just forbid this case? OSMO_ASSERT(n > 0)? This way no one can accidently do a
printf on the text and crash?
- if (c == 0x1b && i + 1 < septet_l) {
+ c8 = gsm_7bit_alphabet[0x7f + c7];
+ } else if (c7 == 0x1b && i + 1 < septet_l) {
next_is_ext = 1;
+ continue;
we try to avoid continue/break inside if statements. I have no counter proposal for
the code flow though. :}
+ nchars = (int)text - (int)text_buf_begin;
I thought you wanted to use ptrdiff_t here? I think we should not truncate a 64bit
point to potentially 32bit.
-int gsm_7bit_encode_oct(uint8_t *result, const char
*data, int *octets)
+int gsm_7bit_encode_n(uint8_t *result, size_t n, const char *data, int *octets)
{
int y = 0;
+ int o;
+ int max_septets = n * 8 / 7;
/* prepare for the worst case, every character expanding to two bytes */
uint8_t *rdata = calloc(strlen(data) * 2, sizeof(uint8_t));
while we are here we could add the null check. and null terminate the result?
y = gsm_septet_encode(rdata, data);
- *octets = gsm_septets2octets(result, rdata, y, 0);
+
+ if (y > max_septets)
+ y = max_septets;
when would this happen? does this break an assumption of our code?
+int gsm_7bit_encode_n_ussd(uint8_t *result, size_t n,
const char *data, int *octets)
+{
+ int y;
+
+ y = gsm_7bit_encode_n(result, n, data, octets);
+ /* if last octet contains only one bit, add <CR> */
+ if (((y * 7) & 7) == 1)
+ result[(*octets) - 1] |= ('\r' << 1);
okay, y >= 1 => *octets >= 1. But shouldn't we increase y in this case too?
+ .input = (const uint8_t *)concatenated_text,
i think we tend to put a ' ' after the ')'. But thanks for fixing all the
warnings GCC started to emit.
holger
[1]
http://www.stack.nl/~dimitri/doxygen/manual/grouping.html