This function works like osmo_hexdump() and return a static buffer containing hex bytes along with markes for the layers.
Sponsored-by: On-Waves ehf --- include/osmocom/core/msgb.h | 1 + src/msgb.c | 48 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+)
diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index c4be0c3..33e8081 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -73,6 +73,7 @@ extern void msgb_enqueue(struct llist_head *queue, struct msgb *msg); extern struct msgb *msgb_dequeue(struct llist_head *queue); extern void msgb_reset(struct msgb *m); uint16_t msgb_length(const struct msgb *msg); +extern const char *msgb_hexdump(const struct msgb *msg);
#ifdef MSGB_DEBUG #include <osmocom/core/panic.h> diff --git a/src/msgb.c b/src/msgb.c index 359a545..98c7d79 100644 --- a/src/msgb.c +++ b/src/msgb.c @@ -153,4 +153,52 @@ void msgb_set_talloc_ctx(void *ctx) tall_msgb_ctx = ctx; }
+/*! \brief Return (static) buffer containing a hexdump of the msg + * \param[in] msg message buffer + * \returns a pointer to a static char array + */ +const char *msgb_hexdump(const struct msgb *msg) +{ + static char buf[4100]; + int buf_offs = 0; + int nchars; + const unsigned char *start = msg->data; + const unsigned char *lxhs[4]; + int i; + + lxhs[0] = msg->l1h; + lxhs[1] = msg->l2h; + lxhs[2] = msg->l3h; + lxhs[3] = msg->l4h; + + for (i = 0; i < ARRAY_SIZE(lxhs); i++) { + if (lxhs[i]) { + if (lxhs[i] < msg->data) + goto out_of_range; + if (lxhs[i] > msg->tail) + goto out_of_range; + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "%s[L%d]> ", + osmo_hexdump(start, lxhs[i] - start), + i+1); + if (nchars < 0 || nchars + buf_offs >= sizeof(buf)) + return "ERROR"; + + buf_offs += nchars; + start = lxhs[i]; + } + } + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "%s", osmo_hexdump(start, msg->tail - start)); + if (nchars < 0 || nchars + buf_offs >= sizeof(buf)) + return "ERROR"; + + return buf; + +out_of_range: + nchars = snprintf(buf, sizeof(buf) - buf_offs, + "!!! L%d out of range", i+1); + return buf; +} + /*! @} */
Writing directly to following struct fields may cause inconsistencies that hard to debug:
data_len, len, head, tail, data
In general, the available macros and functions should be used to modify them instead.
This patch declares these fields as const if MSGB_DISABLE_DIRECT_WRITE is defined. Doing so may lead to warnings and errors, therefore this macro is only defined for libosmocore yet, where at least the errors are also fixed by this patch.
The main purpose is to maintain consistency, so only modifing the fields themselves is restricted. It's still possible to modify the data the pointers refer to.
Sponsored-by: On-Waves ehf --- Doxyfile.core.in | 2 +- Makefile.am | 3 +- include/osmocom/core/msgb.h | 69 ++++++++++++++++++++++++++++++------------ src/msgb.c | 20 ++++++------ tests/gsm0808/gsm0808_test.c | 3 +- 5 files changed, 63 insertions(+), 34 deletions(-)
diff --git a/Doxyfile.core.in b/Doxyfile.core.in index 71843cc..eecd123 100644 --- a/Doxyfile.core.in +++ b/Doxyfile.core.in @@ -1447,7 +1447,7 @@ INCLUDE_FILE_PATTERNS = # undefined via #undef or recursively expanded use the := operator # instead of the = operator.
-PREDEFINED = +PREDEFINED = DOXYGEN
# If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then # this tag can be used to specify a list of macro names that should be expanded. diff --git a/Makefile.am b/Makefile.am index a77bd8f..cb194dc 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1,6 +1,7 @@ ACLOCAL_AMFLAGS = -I m4
-AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include +export AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include -DMSGB_DISABLE_DIRECT_WRITE + SUBDIRS = include src src/vty src/codec src/gsm src/gb tests utils
pkgconfigdir = $(libdir)/pkgconfig diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index 33e8081..f627164 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -38,6 +38,12 @@
#define MSGB_DEBUG
+#ifdef MSGB_DISABLE_DIRECT_WRITE +# define MSGB_CONST const +#else +# define MSGB_CONST +#endif + /*! \brief Osmocom message buffer */ struct msgb { struct llist_head list; /*!< \brief linked list header */ @@ -58,12 +64,35 @@ struct msgb {
unsigned long cb[5]; /*!< \brief control buffer */
- uint16_t data_len; /*!< \brief length of underlying data array */ - uint16_t len; /*!< \brief length of bytes used in msgb */ +#ifdef DOXYGEN + const uint16_t data_len; /*!< \brief length of underlying data array */ + const uint16_t len; /*!< \brief length of bytes used in msgb */ + unsigned char const *head; /*!< \brief start of underlying memory buffer */ + unsigned char const *tail; /*!< \brief end of message in buffer */ + unsigned char const *data; /*!< \brief start of message in buffer */ +#else + union { + MSGB_CONST uint16_t data_len; + uint16_t __data_len; + }; + union { + MSGB_CONST uint16_t len; + uint16_t __len; + }; + union { + unsigned char * MSGB_CONST head; + unsigned char *__head; + }; + union { + unsigned char * MSGB_CONST tail; + unsigned char *__tail; + }; + union { + unsigned char * MSGB_CONST data; + unsigned char *__data; + }; +#endif
- unsigned char *head; /*!< \brief start of underlying memory buffer */ - unsigned char *tail; /*!< \brief end of message in buffer */ - unsigned char *data; /*!< \brief start of message in buffer */ unsigned char _data[0]; /*!< \brief optional immediate data array */ };
@@ -179,12 +208,12 @@ static inline int msgb_headroom(const struct msgb *msgb) */ static inline unsigned char *msgb_put(struct msgb *msgb, unsigned int len) { - unsigned char *tmp = msgb->tail; + unsigned char *tmp = msgb->__tail; if (msgb_tailroom(msgb) < (int) len) MSGB_ABORT(msgb, "Not enough tailroom msgb_push (%u < %u)\n", msgb_tailroom(msgb), len); - msgb->tail += len; - msgb->len += len; + msgb->__tail += len; + msgb->__len += len; return tmp; }
@@ -228,12 +257,12 @@ static inline void msgb_put_u32(struct msgb *msgb, uint32_t word) */ static inline unsigned char *msgb_get(struct msgb *msgb, unsigned int len) { - unsigned char *tmp = msgb->data - len; + unsigned char *tmp = msgb->__data - len; if (msgb_length(msgb) < len) MSGB_ABORT(msgb, "msgb too small to get %u (len %u)\n", len, msgb_length(msgb)); - msgb->tail -= len; - msgb->len -= len; + msgb->__tail -= len; + msgb->__len -= len; return tmp; } /*! \brief remove uint8 from end of message @@ -281,9 +310,9 @@ static inline unsigned char *msgb_push(struct msgb *msgb, unsigned int len) if (msgb_headroom(msgb) < (int) len) MSGB_ABORT(msgb, "Not enough headroom msgb_push (%u < %u)\n", msgb_headroom(msgb), len); - msgb->data -= len; - msgb->len += len; - return msgb->data; + msgb->__data -= len; + msgb->__len += len; + return msgb->__data; } /*! \brief remove (pull) a header from the front of the message buffer * \param[in] msgb message buffer @@ -296,8 +325,8 @@ static inline unsigned char *msgb_push(struct msgb *msgb, unsigned int len) */ static inline unsigned char *msgb_pull(struct msgb *msgb, unsigned int len) { - msgb->len -= len; - return msgb->data += len; + msgb->__len -= len; + return msgb->__data += len; }
/*! \brief remove (pull) all headers in front of l3h from the message buffer. @@ -356,8 +385,8 @@ static inline uint32_t msgb_pull_u32(struct msgb *msgb) */ static inline void msgb_reserve(struct msgb *msg, int len) { - msg->data += len; - msg->tail += len; + msg->__data += len; + msg->__tail += len; }
/*! \brief Trim the msgb to a given absolute length @@ -370,8 +399,8 @@ static inline int msgb_trim(struct msgb *msg, int len) if (len > msg->data_len) return -1;
- msg->len = len; - msg->tail = msg->data + len; + msg->__len = len; + msg->__tail = msg->__data + len;
return 0; } diff --git a/src/msgb.c b/src/msgb.c index 98c7d79..a3a48d9 100644 --- a/src/msgb.c +++ b/src/msgb.c @@ -55,11 +55,11 @@ struct msgb *msgb_alloc(uint16_t size, const char *name) return NULL; }
- msg->data_len = size; - msg->len = 0; - msg->data = msg->_data; - msg->head = msg->_data; - msg->tail = msg->_data; + msg->__data_len = size; + msg->__len = 0; + msg->__data = msg->_data; + msg->__head = msg->_data; + msg->__tail = msg->_data;
return msg; } @@ -113,10 +113,10 @@ struct msgb *msgb_dequeue(struct llist_head *queue) */ void msgb_reset(struct msgb *msg) { - msg->len = 0; - msg->data = msg->_data; - msg->head = msg->_data; - msg->tail = msg->_data; + msg->__len = 0; + msg->__data = msg->_data; + msg->__head = msg->_data; + msg->__tail = msg->_data;
msg->trx = NULL; msg->lchan = NULL; @@ -133,7 +133,7 @@ void msgb_reset(struct msgb *msg) */ uint8_t *msgb_data(const struct msgb *msg) { - return msg->data; + return msg->__data; }
/*! \brief get length of message buffer diff --git a/tests/gsm0808/gsm0808_test.c b/tests/gsm0808/gsm0808_test.c index 7e5e97b..1ce6ef9 100644 --- a/tests/gsm0808/gsm0808_test.c +++ b/tests/gsm0808/gsm0808_test.c @@ -108,8 +108,7 @@ static void test_create_cipher_complete() msgb_free(msg);
/* with l3 data but short */ - l3->len -= 1; - l3->tail -= 1; + msgb_trim(l3, l3->len - 1); msg = gsm0808_create_cipher_complete(l3, 4); VERIFY(msg, res2, ARRAY_SIZE(res2)); msgb_free(msg);
On Fri, Feb 28, 2014 at 08:47:46PM +0100, Jacob Erlbeck wrote:
Good Morning,
I welcome any change that makes it more difficult to create the mess that LAPD/LAPDm is in. I have some questions and comments though.
Writing directly to following struct fields may cause inconsistencies that hard to debug:
In general, the available macros and functions should be used to modify them instead.
^ must as in RFC?
This patch declares these fields as const if MSGB_DISABLE_DIRECT_WRITE is defined. Doing so may lead to warnings and errors, therefore this macro is only defined for libosmocore yet, where at least the errors are also fixed by this patch.
I will get back to this later as well. Have you looked at GSEAL of GTK+ 2.0? Their goal was to hide direct access to variables too and they introduced a macro like:
#ifndef GSEAL /* introduce GSEAL() here for all of Gdk and Gtk+ without the need to modify GLib */ # ifdef GSEAL_ENABLE # define GSEAL(ident) _g_sealed__ ## ident # else # define GSEAL(ident) ident # endif #endif /* !GSEAL */
It is not making the variables const but hiding them with a rename and causing a compilation failure when being accessed.
-AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include +export AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include -DMSGB_DISABLE_DIRECT_WRITE
Is that really necessary?
+#ifdef DOXYGEN
My take would be that we try to avoid direct access so we don't need to document these variables and can use \internal here. I think it is preferable to avoid the situation where the ABI changes based on an external define (even if it is just for documentation now). I have seen a lot of wild CFLAGS/CPPFLAGS that users set while working on OpenEmbedded
- const uint16_t data_len; /*!< \brief length of underlying data array */
- const uint16_t len; /*!< \brief length of bytes used in msgb */
- unsigned char const *head; /*!< \brief start of underlying memory buffer */
- unsigned char const *tail; /*!< \brief end of message in buffer */
- unsigned char const *data; /*!< \brief start of message in buffer */
+#else
- union {
MSGB_CONST uint16_t data_len;uint16_t __data_len;- };
Anyone knows an ABI where alignment for const/non-const is different? :)
cheers holger
Hi,
On 02.03.2014 08:24, Holger Hans Peter Freyther wrote:
On Fri, Feb 28, 2014 at 08:47:46PM +0100, Jacob Erlbeck wrote:
Writing directly to following struct fields may cause inconsistencies that hard to debug:
In general, the available macros and functions should be used to modify them instead.
^ must as in RFC?
Rather "should" as in RFC2119: SHOULD This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course.
But I don't really see many 'particular circumstances' that would justify that. Perhaps something like: It's impossible to achieve with the existing API _and_ the current libosmocore must be used _and_ the new functionality is wrapped in a new macro/function written specifically for that purpose.
Or did you mean "MUST (...)" as in RFC6919 ;-)
I will get back to this later as well. Have you looked at GSEAL of GTK+ 2.0? Their goal was to hide direct access to variables too and they introduced a macro like:
#ifndef GSEAL /* introduce GSEAL() here for all of Gdk and Gtk+ without the need to modify GLib */ # ifdef GSEAL_ENABLE # define GSEAL(ident) _g_sealed__ ## ident # else # define GSEAL(ident) ident # endif #endif /* !GSEAL */
It is not making the variables const but hiding them with a rename and causing a compilation failure when being accessed.
I'm only addressing write access (yet), so sealing wouldn't help here. That would be different, if we was forbidding direct access to the fields completely, e.g. to make certain optimizations possible.
-AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include +export AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include -DMSGB_DISABLE_DIRECT_WRITE
Is that really necessary?
AM_CPPFLAGS has not been passed to the sub-makes and thus wasn't honoured at all. But I'll remove the 'include' stuff from that variable, because it had worked pretty well without them so far.
+#ifdef DOXYGEN
My take would be that we try to avoid direct access so we don't need to document these variables and can use \internal here. I think it is preferable to avoid the situation where the ABI changes based on an external define (even if it is just for documentation now). I have seen a lot of wild CFLAGS/CPPFLAGS that users set while working on OpenEmbedded
Ok, I didn't want to break the Doxygen output, but having inconsistent variants is worse (it's already broken, see the 'const'). I'll gladly remove it.
Anyone knows an ABI where alignment for const/non-const is different? :)
It's a qualifier only, and according to C99, 6.2.5 (23):
"the qualified or unqualified versions of a type are distinct types that belong to the same type category and have the same representation and alignment requirements." and "The same representation and alignment requirements are meant to imply interchangeability as arguments to functions, return values from functions, and members of unions."
Jacob
On Fri, Feb 28, 2014 at 08:47:45PM +0100, Jacob Erlbeck wrote:
Good morning Jacob,
as usual thank you very much for your work.
This function works like osmo_hexdump() and return a static buffer
^ returns
containing hex bytes along with markes for the layers.
^ markers
+/*! \brief Return (static) buffer containing a hexdump of the msg
^ Return "a"
+const char *msgb_hexdump(const struct msgb *msg) +{
- static char buf[4100];
- int buf_offs = 0;
- int nchars;
- const unsigned char *start = msg->data;
- const unsigned char *lxhs[4];
- int i;
- lxhs[0] = msg->l1h;
- lxhs[1] = msg->l2h;
- lxhs[2] = msg->l3h;
- lxhs[3] = msg->l4h;
- for (i = 0; i < ARRAY_SIZE(lxhs); i++) {
if (lxhs[i]) {
if (!lxhs[i]) continue;
This avoids having the show at three levels of indention in.
nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs,"%s[L%d]> ",osmo_hexdump(start, lxhs[i] - start),i+1);
- nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs,
"%s", osmo_hexdump(start, msg->tail - start));- nchars = snprintf(buf, sizeof(buf) - buf_offs,
"!!! L%d out of range", i+1);
So sizeof(buf) - buf_offs can only be 0 when the output is already null terminated?
kind regards holger
Dear Holger
On 02.03.2014 08:06, Holger Hans Peter Freyther wrote:
nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs,"%s[L%d]> ",osmo_hexdump(start, lxhs[i] - start),i+1);
- nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs,
"%s", osmo_hexdump(start, msg->tail - start));- nchars = snprintf(buf, sizeof(buf) - buf_offs,
"!!! L%d out of range", i+1);So sizeof(buf) - buf_offs can only be 0 when the output is already null terminated?
No, the expressions can never be 0 due to if (nchars < 0 || nchars + buf_offs >= sizeof(buf)) return "ERROR";
But even if it could, the string has already been \0 terminated by prior calls to snprintf (the initial size value is always > 0).
Jacob
kind regards holger
This function works like osmo_hexdump() and returns a static buffer containing hex bytes along with markers for the layers.
Note that it uses osmo_hexdump() internally, thus a call to msgb_hexdump() invalidates the buffer that has been returned by an earlier call to osmo_hexdump(). In short: don't mix them in a single call printf().
Sponsored-by: On-Waves ehf --- include/osmocom/core/msgb.h | 1 + src/msgb.c | 49 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)
diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index fe2733b..5d4bd84 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -73,6 +73,7 @@ extern void msgb_enqueue(struct llist_head *queue, struct msgb *msg); extern struct msgb *msgb_dequeue(struct llist_head *queue); extern void msgb_reset(struct msgb *m); uint16_t msgb_length(const struct msgb *msg); +extern const char *msgb_hexdump(const struct msgb *msg);
#ifdef MSGB_DEBUG #include <osmocom/core/panic.h> diff --git a/src/msgb.c b/src/msgb.c index 359a545..b2fe1d2 100644 --- a/src/msgb.c +++ b/src/msgb.c @@ -153,4 +153,53 @@ void msgb_set_talloc_ctx(void *ctx) tall_msgb_ctx = ctx; }
+/*! \brief Return a (static) buffer containing a hexdump of the msg + * \param[in] msg message buffer + * \returns a pointer to a static char array + */ +const char *msgb_hexdump(const struct msgb *msg) +{ + static char buf[4100]; + int buf_offs = 0; + int nchars; + const unsigned char *start = msg->data; + const unsigned char *lxhs[4]; + int i; + + lxhs[0] = msg->l1h; + lxhs[1] = msg->l2h; + lxhs[2] = msg->l3h; + lxhs[3] = msg->l4h; + + for (i = 0; i < ARRAY_SIZE(lxhs); i++) { + if (!lxhs[i]) + continue; + + if (lxhs[i] < msg->data) + goto out_of_range; + if (lxhs[i] > msg->tail) + goto out_of_range; + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "%s[L%d]> ", + osmo_hexdump(start, lxhs[i] - start), + i+1); + if (nchars < 0 || nchars + buf_offs >= sizeof(buf)) + return "ERROR"; + + buf_offs += nchars; + start = lxhs[i]; + } + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "%s", osmo_hexdump(start, msg->tail - start)); + if (nchars < 0 || nchars + buf_offs >= sizeof(buf)) + return "ERROR"; + + return buf; + +out_of_range: + nchars = snprintf(buf, sizeof(buf) - buf_offs, + "!!! L%d out of range", i+1); + return buf; +} + /*! @} */
This adds and uses a wrapper for lapdm_phsap_dequeue_prim() that prints information about the message that has been taken from the queue. --- tests/lapd/lapd_test.c | 86 ++++++++++++++++++++++++++++++++--------------- tests/lapd/lapd_test.ok | 18 +++++++--- 2 files changed, 72 insertions(+), 32 deletions(-)
diff --git a/tests/lapd/lapd_test.c b/tests/lapd/lapd_test.c index b4594de..1842ab7 100644 --- a/tests/lapd/lapd_test.c +++ b/tests/lapd/lapd_test.c @@ -240,6 +240,51 @@ static int ms_to_bts_l1_cb(struct osmo_prim_hdr *oph, void *_ctx) return rc; }
+static int dequeue_prim(struct lapdm_entity *le, struct osmo_phsap_prim *pp, + const char *queue_name) +{ + int rc; + int l2_header_len; + int l3_len = 0; + + /* Take message from queue */ + rc = lapdm_phsap_dequeue_prim(le, pp); + + fprintf(stderr, "dequeue: got rc %d: %s\n", rc, + rc <= 0 ? strerror(-rc) : "-"); + + if (rc < 0) + return rc; + + l2_header_len = msgb_l2len(pp->oph.msg); + if (msgb_l3(pp->oph.msg)) { + l3_len = msgb_l3len(pp->oph.msg); + l2_header_len -= l3_len; + } else + fprintf(stderr, "MSGB: L3 is undefined\n"); + + if (l2_header_len < 0 || l2_header_len > pp->oph.msg->data_len) { + fprintf(stderr, + "MSGB inconsistent: data = %p, l2 = %p, l3 = %p, tail = %p\n", + pp->oph.msg->data, + pp->oph.msg->l2h, + pp->oph.msg->l3h, + pp->oph.msg->tail); + l2_header_len = -1; + } + + printf("Took message from %s queue: " + "L2 header size %d, L3 size %d, " + "SAP %#x, %d/%d, Link 0x%02x\n", + queue_name, + l2_header_len, l3_len, + pp->oph.sap, pp->oph.primitive, pp->oph.operation, + pp->u.data.link_id); + printf("Message: %s\n", msgb_hexdump(pp->oph.msg)); + + return rc; +} + static int ms_to_bts_tx_cb(struct msgb *msg, struct lapdm_entity *le, void *_ctx) { struct lapdm_polling_state *state = _ctx; @@ -315,7 +360,7 @@ static void test_lapdm_polling() /* 2. Poll on the BTS for sending out a confirmation */ printf("\nConfirming\n"); OSMO_ASSERT(test_state.bts_read == 1); - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); CHECK_RC(rc); OSMO_ASSERT(pp.oph.msg->data == pp.oph.msg->l2h); send(pp.oph.msg, &ms_to_bts_channel); @@ -325,14 +370,14 @@ static void test_lapdm_polling() /* 3. Send some data to the MS */ printf("\nSending back to MS\n"); lapdm_rslms_recvmsg(create_mm_id_req(), &bts_to_ms_channel); - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); CHECK_RC(rc); send(pp.oph.msg, &ms_to_bts_channel); msgb_free(pp.oph.msg); OSMO_ASSERT(test_state.ms_read == 2);
/* verify that there is nothing more to poll */ - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); OSMO_ASSERT(rc < 0);
/* 3. And back to the BTS */ @@ -344,14 +389,14 @@ static void test_lapdm_polling() /* 4. And back to the MS, but let's move data/l2h apart */ OSMO_ASSERT(test_state.bts_read == 2); OSMO_ASSERT(test_state.ms_read == 2); - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); CHECK_RC(rc); send(pp.oph.msg, &ms_to_bts_channel); OSMO_ASSERT(test_state.ms_read == 2); msgb_free(pp.oph.msg);
/* verify that there is nothing more to poll */ - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); OSMO_ASSERT(rc < 0);
/* check sending an empty L3 message fails */ @@ -392,18 +437,18 @@ static void test_lapdm_contention_resolution()
/* Send SABM MS 1, we must get UA */ send_sabm(&bts_to_ms_channel, 0); - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); CHECK_RC(rc); OSMO_ASSERT(memcmp(pp.oph.msg->l2h, ua, ARRAY_SIZE(ua)) == 0);
/* Send SABM MS 2, we must get nothing, due to collision */ send_sabm(&bts_to_ms_channel, 1); - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); OSMO_ASSERT(rc == -ENODEV);
/* Send SABM MS 1 again, we must get UA gain */ send_sabm(&bts_to_ms_channel, 0); - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); CHECK_RC(rc); OSMO_ASSERT(memcmp(pp.oph.msg->l2h, ua, ARRAY_SIZE(ua)) == 0);
@@ -451,7 +496,6 @@ static void lapdm_establish(const uint8_t *est_req, size_t est_req_size) struct lapdm_polling_state test_state; struct osmo_phsap_prim pp; struct msgb *msg; - const char *queue_name;
/* Configure LAPDm on both sides */ struct lapdm_channel bts_to_ms_channel; @@ -473,31 +517,17 @@ static void lapdm_establish(const uint8_t *est_req, size_t est_req_size) OSMO_ASSERT(rc == 0);
/* Take message from queue */ - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); - if (rc >= 0) - queue_name = "DCCH"; - else { - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_acch, &pp); - if (rc >= 0) - queue_name = "ACCH"; - } + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); + if (rc < 0) + rc = dequeue_prim(&bts_to_ms_channel.lapdm_acch, &pp, "ACCH");
- fprintf(stderr, "dequeue: got rc %d: %s\n", rc, - rc <= 0 ? strerror(-rc) : "-"); CHECK_RC(rc);
- printf("Took message from %s queue: L2 header size %d, " - "SAP %#x, %d/%d, Link 0x%02x\n", - queue_name, msgb_l2len(pp.oph.msg) - msgb_l3len(pp.oph.msg), - pp.oph.sap, pp.oph.primitive, pp.oph.operation, - pp.u.data.link_id); - printf("Message: %s\n", osmo_hexdump(pp.oph.msg->data, pp.oph.msg->len)); - OSMO_ASSERT(pp.oph.msg->data == msgb_l2(pp.oph.msg));
- rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); OSMO_ASSERT(rc < 0); - rc = lapdm_phsap_dequeue_prim(&bts_to_ms_channel.lapdm_acch, &pp); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_acch, &pp, "ACCH"); OSMO_ASSERT(rc < 0);
/* clean up */ diff --git a/tests/lapd/lapd_test.ok b/tests/lapd/lapd_test.ok index e4b1359..9fb58e0 100644 --- a/tests/lapd/lapd_test.ok +++ b/tests/lapd/lapd_test.ok @@ -5,10 +5,14 @@ bts_to_ms_tx_cb: MS->BTS(us) message 25 BTS: Verifying CM request.
Confirming +Took message from DCCH queue: L2 header size 3, L3 size 20, SAP 0x1000000, 0/0, Link 0x00 +Message: [L2]> 01 73 41 [L3]> 05 24 31 03 50 18 93 08 29 47 80 00 00 00 00 80 2b 2b 2b 2b ms_to_bts_tx_cb: BTS->MS(us) message 9 MS: Verifying incoming primitive.
Sending back to MS +Took message from DCCH queue: L2 header size 3, L3 size 20, SAP 0x1000000, 0/0, Link 0x00 +Message: [L2]> 03 00 0d [L3]> 05 04 0d 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b ms_to_bts_tx_cb: BTS->MS(us) message 12 MS: Verifying incoming MM message: 3 ms_to_bts_l1_cb: MS(us) -> BTS prim message @@ -17,15 +21,21 @@ Sending back to BTS ms_to_bts_l1_cb: MS(us) -> BTS prim message bts_to_ms_tx_cb: MS->BTS(us) message 14 BTS: Verifying dummy message. +Took message from DCCH queue: L2 header size 23, L3 size 0, SAP 0x1000000, 0/0, Link 0x00 +Message: [L2]> 01 21 01 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b I test RF channel release of an unestablished channel. I test contention resultion by having two mobiles collide and first mobile repeating SABM. bts_to_ms_tx_cb: MS->BTS(us) message 25 BTS: Verifying CM request. +Took message from DCCH queue: L2 header size 3, L3 size 20, SAP 0x1000000, 0/0, Link 0x00 +Message: [L2]> 01 73 41 [L3]> 05 24 31 03 50 18 93 08 29 47 80 00 00 00 00 80 2b 2b 2b 2b +Took message from DCCH queue: L2 header size 3, L3 size 20, SAP 0x1000000, 0/0, Link 0x00 +Message: [L2]> 01 73 41 [L3]> 05 24 31 03 50 18 93 08 29 47 80 00 00 00 00 80 2b 2b 2b 2b I test RF channel establishment. Testing SAPI3/SDCCH -Took message from DCCH queue: L2 header size 3, SAP 0x1000000, 0/0, Link 0x03 -Message: 0f 3f 01 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b +Took message from DCCH queue: L2 header size 3, L3 size 20, SAP 0x1000000, 0/0, Link 0x03 +Message: [L2]> 0f 3f 01 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b Testing SAPI3/SACCH -Took message from ACCH queue: L2 header size 5, SAP 0x1000000, 0/0, Link 0x43 -Message: 00 00 0f 3f 01 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b +Took message from ACCH queue: L2 header size 5, L3 size 18, SAP 0x1000000, 0/0, Link 0x43 +Message: [L2]> 00 00 0f 3f 01 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b Success.
Since e.g. the IPA input driver leaves it's specific header in front of msg->l2h, so that msg->l2h != msg->data. The lapdm code does not expect this at least in rslms_rx_rll_est_req().
This patch modifies the test program to add a dummy L1 header to generated messages (unless the test would abort when doing so).
Note that the ok file reflects the current state which is not correct.
Sponsored-by: On-Waves ehf --- tests/lapd/lapd_test.c | 12 ++++++++++++ tests/lapd/lapd_test.ok | 8 ++++---- 2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/tests/lapd/lapd_test.c b/tests/lapd/lapd_test.c index 1842ab7..e3d4e80 100644 --- a/tests/lapd/lapd_test.c +++ b/tests/lapd/lapd_test.c @@ -36,6 +36,7 @@ }
static struct log_info info = {}; +static int dummy_l1_header_len = 0;
struct lapdm_polling_state { struct lapdm_channel *bts; @@ -94,6 +95,7 @@ static struct msgb *create_cm_serv_req(void)
msg = msgb_from_array(cm, sizeof(cm)); rsl_rll_push_l3(msg, RSL_MT_EST_REQ, 0, 0, 1); + msgb_push(msg, dummy_l1_header_len); return msg; }
@@ -106,6 +108,7 @@ static struct msgb *create_mm_id_req(void) OSMO_ASSERT(msgb_l2len(msg) == 12); msg->l3h = msg->l2h + 6; OSMO_ASSERT(msgb_l3len(msg) == 6); + msgb_push(msg, dummy_l1_header_len);
return msg; } @@ -117,6 +120,7 @@ static struct msgb *create_empty_msg(void) msg = msgb_from_array(NULL, 0); OSMO_ASSERT(msgb_l3len(msg) == 0); rsl_rll_push_l3(msg, RSL_MT_DATA_REQ, 0, 0, 1); + msgb_push(msg, dummy_l1_header_len); return msg; }
@@ -126,6 +130,7 @@ static struct msgb *create_dummy_data_req(void)
msg = msgb_from_array(dummy1, sizeof(dummy1)); rsl_rll_push_l3(msg, RSL_MT_DATA_REQ, 0, 0, 1); + msgb_push(msg, dummy_l1_header_len); return msg; }
@@ -135,6 +140,7 @@ static struct msgb *create_rel_req(void)
msg = msgb_from_array(rel_req, sizeof(rel_req)); msg->l2h = msg->data; + msgb_push(msg, dummy_l1_header_len); msg->l3h = msg->l2h + sizeof(struct abis_rsl_rll_hdr); return msg; } @@ -145,6 +151,7 @@ static struct msgb *create_est_req(const uint8_t *est_req, size_t est_req_size)
msg = msgb_from_array(est_req, est_req_size); msg->l2h = msg->data; + msgb_push(msg, dummy_l1_header_len); msg->l3h = msg->l2h + sizeof(struct abis_rsl_rll_hdr); return msg; } @@ -550,10 +557,15 @@ int main(int argc, char **argv) { osmo_init_logging(&info);
+ /* Prevent the test from segfaulting */ + dummy_l1_header_len = 0; test_lapdm_polling(); + + dummy_l1_header_len = 3; test_lapdm_early_release(); test_lapdm_contention_resolution(); test_lapdm_establishment(); + printf("Success.\n");
return 0; diff --git a/tests/lapd/lapd_test.ok b/tests/lapd/lapd_test.ok index 9fb58e0..7d266bd 100644 --- a/tests/lapd/lapd_test.ok +++ b/tests/lapd/lapd_test.ok @@ -33,9 +33,9 @@ Took message from DCCH queue: L2 header size 3, L3 size 20, SAP 0x1000000, 0/0, Message: [L2]> 01 73 41 [L3]> 05 24 31 03 50 18 93 08 29 47 80 00 00 00 00 80 2b 2b 2b 2b I test RF channel establishment. Testing SAPI3/SDCCH -Took message from DCCH queue: L2 header size 3, L3 size 20, SAP 0x1000000, 0/0, Link 0x03 -Message: [L2]> 0f 3f 01 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b +Took message from DCCH queue: L2 header size 6, L3 size 17, SAP 0x1000000, 0/0, Link 0x03 +Message: [L2]> 0f 3f 0d 20 02 03 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b Testing SAPI3/SACCH -Took message from ACCH queue: L2 header size 5, L3 size 18, SAP 0x1000000, 0/0, Link 0x43 -Message: [L2]> 00 00 0f 3f 01 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b +Took message from ACCH queue: L2 header size 8, L3 size 15, SAP 0x1000000, 0/0, Link 0x43 +Message: [L2]> 00 00 0f 3f 0d 0b 02 43 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b Success.
This just adds a single test to verify that the ACCH queue is actually empty.
Sponsored-by: On-Waves ehf --- tests/lapd/lapd_test.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tests/lapd/lapd_test.c b/tests/lapd/lapd_test.c index e3d4e80..6b5cfd3 100644 --- a/tests/lapd/lapd_test.c +++ b/tests/lapd/lapd_test.c @@ -405,6 +405,8 @@ static void test_lapdm_polling() /* verify that there is nothing more to poll */ rc = dequeue_prim(&bts_to_ms_channel.lapdm_dcch, &pp, "DCCH"); OSMO_ASSERT(rc < 0); + rc = dequeue_prim(&bts_to_ms_channel.lapdm_acch, &pp, "ACCH"); + OSMO_ASSERT(rc < 0);
/* check sending an empty L3 message fails */ rc = lapdm_rslms_recvmsg(create_empty_msg(), &bts_to_ms_channel);
Currently it takes 3s to establish a SAPI 3 SACCH connection with osmo-bts. This is due to the fact, that a broken SABME request is sent first and and is ignored by the MS. Then, after a T200 timeout (2s) the SABME command is sent again (this time correctly) and answered by the MS.
The first SABME message is broken (it has a length field of 3 and ends with 3 bytes from the tail of the original RSL message), because of it is expected throughout lapdm.c that msg buffers containing RSL have msg->l2h == msg->data. Some abis input drivers fulfill this but IPA doesn't, thus the 3 bytes of the IPA header are still part of the msg and confuse length computation.
Since internal fields of the msg are modified directly, this is difficult to see.
This patch adds a new function msgb_pull_to_l3() that explicitely skips over all headers prepending L3 and therefore resets l1h and l2h. This function is then used instead of msgb_pull_l2h() which only worked correctly when msg->l2h == msg->data. In addition, code manipulating msg->tail and msg->len directly has been replaced by calls to msgb_trim().
Note that this patch does not fix all issues of this case in the LADP related code.
Ticket: #192 Sponsored-by: On-Waves ehf --- include/osmocom/core/msgb.h | 15 +++++++++++++++ src/gsm/lapdm.c | 41 ++++++++++++++--------------------------- tests/lapd/lapd_test.ok | 8 ++++---- 3 files changed, 33 insertions(+), 31 deletions(-)
diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index 5d4bd84..33e8081 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -300,6 +300,21 @@ static inline unsigned char *msgb_pull(struct msgb *msgb, unsigned int len) return msgb->data += len; }
+/*! \brief remove (pull) all headers in front of l3h from the message buffer. + * \param[in] msgb message buffer with a valid l3h + * \returns pointer to new start of msgb (l3h) + * + * This function moves the \a data pointer of the \ref msgb further back + * in the message, thereby shrinking the size of the message. + * l1h and l2h will be cleared. + */ +static inline unsigned char *msgb_pull_to_l3(struct msgb *msg) +{ + unsigned char *ret = msgb_pull(msg, msg->l3h - msg->data); + msg->l1h = msg->l2h = NULL; + return ret; +} + /*! \brief remove uint8 from front of message * \param[in] msgb message buffer * \returns 8bit value taken from end of msgb diff --git a/src/gsm/lapdm.c b/src/gsm/lapdm.c index 19f78a1..41f4be7 100644 --- a/src/gsm/lapdm.c +++ b/src/gsm/lapdm.c @@ -193,14 +193,6 @@ static struct lapdm_datalink *datalink_for_sapi(struct lapdm_entity *le, uint8_t } }
-/* remove the L2 header from a MSGB */ -static inline unsigned char *msgb_pull_l2h(struct msgb *msg) -{ - unsigned char *ret = msgb_pull(msg, msg->l3h - msg->l2h); - msg->l2h = NULL; - return ret; -} - /* Append padding (if required) */ static void lapdm_pad_msgb(struct msgb *msg, uint8_t n201) { @@ -611,7 +603,7 @@ static int l2_ph_data_ind(struct msgb *msg, struct lapdm_entity *le, lctx.length = n201; lctx.more = 0; msg->l3h = msg->l2h + 2; - msgb_pull_l2h(msg); + msgb_pull_to_l3(msg); } else { /* length field */ if (!(msg->l2h[2] & LAPDm_EL)) { @@ -629,7 +621,7 @@ static int l2_ph_data_ind(struct msgb *msg, struct lapdm_entity *le, lctx.length = msg->l2h[2] >> 2; lctx.more = !!(msg->l2h[2] & LAPDm_MORE); msg->l3h = msg->l2h + 3; - msgb_pull_l2h(msg); + msgb_pull_to_l3(msg); } /* store context for messages from lapd */ memcpy(&mctx.dl->mctx, &mctx, sizeof(mctx.dl->mctx)); @@ -644,7 +636,7 @@ static int l2_ph_data_ind(struct msgb *msg, struct lapdm_entity *le, /* directly pass up to layer3 */ LOGP(DLLAPD, LOGL_INFO, "fmt=Bbis UI\n"); msg->l3h = msg->l2h; - msgb_pull_l2h(msg); + msgb_pull_to_l3(msg); rc = send_rslms_rll_l3(RSL_MT_UNIT_DATA_IND, &mctx, msg); break; default: @@ -807,9 +799,8 @@ static int rslms_rx_rll_est_req(struct msgb *msg, struct lapdm_datalink *dl) }
/* Remove RLL header from msgb and set length to L3-info */ - msgb_pull_l2h(msg); - msg->len = length; - msg->tail = msg->l3h + length; + msgb_pull_to_l3(msg); + msgb_trim(msg, length);
/* prepare prim */ osmo_prim_init(&dp.oph, 0, PRIM_DL_EST, PRIM_OP_REQUEST, msg); @@ -861,9 +852,8 @@ static int rslms_rx_rll_udata_req(struct msgb *msg, struct lapdm_datalink *dl) le->tx_power, le->ta);
/* Remove RLL header from msgb and set length to L3-info */ - msgb_pull_l2h(msg); - msg->len = length; - msg->tail = msg->l3h + length; + msgb_pull_to_l3(msg); + msgb_trim(msg, length);
/* Push L1 + LAPDm header on msgb */ msg->l2h = msgb_push(msg, 2 + !ui_bts); @@ -900,9 +890,8 @@ static int rslms_rx_rll_data_req(struct msgb *msg, struct lapdm_datalink *dl) length = TLVP_LEN(&tv, RSL_IE_L3_INFO);
/* Remove RLL header from msgb and set length to L3-info */ - msgb_pull_l2h(msg); - msg->len = length; - msg->tail = msg->l3h + length; + msgb_pull_to_l3(msg); + msgb_trim(msg, length);
/* prepare prim */ osmo_prim_init(&dp.oph, 0, PRIM_DL_DATA, PRIM_OP_REQUEST, msg); @@ -957,9 +946,8 @@ static int rslms_rx_rll_res_req(struct msgb *msg, struct lapdm_datalink *dl) length = TLVP_LEN(&tv, RSL_IE_L3_INFO);
/* Remove RLL header from msgb and set length to L3-info */ - msgb_pull_l2h(msg); - msg->len = length; - msg->tail = msg->l3h + length; + msgb_pull_to_l3(msg); + msgb_trim(msg, length);
/* prepare prim */ osmo_prim_init(&dp.oph, 0, (msg_type == RSL_MT_RES_REQ) ? PRIM_DL_RES @@ -981,12 +969,11 @@ static int rslms_rx_rll_rel_req(struct msgb *msg, struct lapdm_datalink *dl) mode = rllh->data[1] & 1;
/* Pull rllh */ - msgb_pull_l2h(msg); + msgb_pull_to_l3(msg);
/* 04.06 3.8.3: No information field is permitted with the DISC * command. */ - msg->len = 0; - msg->tail = msg->l3h = msg->data; + msgb_trim(msg, 0);
/* prepare prim */ osmo_prim_init(&dp.oph, 0, PRIM_DL_REL, PRIM_OP_REQUEST, msg); @@ -1045,7 +1032,7 @@ static int l2_ph_chan_conf(struct msgb *msg, struct lapdm_entity *le, uint32_t f
gsm_fn2gsmtime(&tm, frame_nr);
- msgb_pull_l2h(msg); + msgb_pull_to_l3(msg); msg->l2h = msgb_push(msg, sizeof(*ch) + sizeof(*ref)); ch = (struct abis_rsl_cchan_hdr *)msg->l2h; rsl_init_cchan_hdr(ch, RSL_MT_CHAN_CONF); diff --git a/tests/lapd/lapd_test.ok b/tests/lapd/lapd_test.ok index 7d266bd..9fb58e0 100644 --- a/tests/lapd/lapd_test.ok +++ b/tests/lapd/lapd_test.ok @@ -33,9 +33,9 @@ Took message from DCCH queue: L2 header size 3, L3 size 20, SAP 0x1000000, 0/0, Message: [L2]> 01 73 41 [L3]> 05 24 31 03 50 18 93 08 29 47 80 00 00 00 00 80 2b 2b 2b 2b I test RF channel establishment. Testing SAPI3/SDCCH -Took message from DCCH queue: L2 header size 6, L3 size 17, SAP 0x1000000, 0/0, Link 0x03 -Message: [L2]> 0f 3f 0d 20 02 03 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b +Took message from DCCH queue: L2 header size 3, L3 size 20, SAP 0x1000000, 0/0, Link 0x03 +Message: [L2]> 0f 3f 01 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b Testing SAPI3/SACCH -Took message from ACCH queue: L2 header size 8, L3 size 15, SAP 0x1000000, 0/0, Link 0x43 -Message: [L2]> 00 00 0f 3f 0d 0b 02 43 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b +Took message from ACCH queue: L2 header size 5, L3 size 18, SAP 0x1000000, 0/0, Link 0x43 +Message: [L2]> 00 00 0f 3f 01 [L3]> 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b Success.
Writing directly to following struct fields may cause inconsistencies that are hard to debug:
data_len, len, head, tail, data
In general, the available macros and functions should be used to modify them instead.
This patch declares these fields as const if MSGB_DISABLE_DIRECT_WRITE is defined. Doing so may lead to warnings and errors, therefore this macro is only defined for libosmocore yet, where at least the errors are also fixed by this patch.
The main purpose is to maintain consistency, so only modifing the fields themselves is restricted. It's still possible to modify the data the pointers refer to.
Sponsored-by: On-Waves ehf --- Doxyfile.core.in | 2 +- Makefile.am | 3 +- include/osmocom/core/msgb.h | 66 +++++++++++++++++++++++++++++------------- src/msgb.c | 20 ++++++------- tests/gsm0808/gsm0808_test.c | 3 +- 5 files changed, 60 insertions(+), 34 deletions(-)
diff --git a/Doxyfile.core.in b/Doxyfile.core.in index 71843cc..eecd123 100644 --- a/Doxyfile.core.in +++ b/Doxyfile.core.in @@ -1447,7 +1447,7 @@ INCLUDE_FILE_PATTERNS = # undefined via #undef or recursively expanded use the := operator # instead of the = operator.
-PREDEFINED = +PREDEFINED = DOXYGEN
# If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then # this tag can be used to specify a list of macro names that should be expanded. diff --git a/Makefile.am b/Makefile.am index a77bd8f..d6a1992 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1,6 +1,7 @@ ACLOCAL_AMFLAGS = -I m4
-AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include +export AM_CPPFLAGS = -DMSGB_DISABLE_DIRECT_WRITE + SUBDIRS = include src src/vty src/codec src/gsm src/gb tests utils
pkgconfigdir = $(libdir)/pkgconfig diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index 33e8081..3593309 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -38,6 +38,12 @@
#define MSGB_DEBUG
+#ifdef MSGB_DISABLE_DIRECT_WRITE +# define MSGB_CONST const +#else +# define MSGB_CONST +#endif + /*! \brief Osmocom message buffer */ struct msgb { struct llist_head list; /*!< \brief linked list header */ @@ -58,12 +64,32 @@ struct msgb {
unsigned long cb[5]; /*!< \brief control buffer */
- uint16_t data_len; /*!< \brief length of underlying data array */ - uint16_t len; /*!< \brief length of bytes used in msgb */ + union { + /*! \brief length of underlying data array */ + MSGB_CONST uint16_t data_len; + uint16_t __data_len; + }; + union { + /*! \brief length of bytes used in msgb */ + MSGB_CONST uint16_t len; + uint16_t __len; + }; + union { + /*! \brief start of underlying memory buffer */ + unsigned char * MSGB_CONST head; + unsigned char *__head; + }; + union { + /*! \brief end of message in buffer */ + unsigned char * MSGB_CONST tail; + unsigned char *__tail; + }; + union { + /*! \brief start of message in buffer */ + unsigned char * MSGB_CONST data; + unsigned char *__data; + };
- unsigned char *head; /*!< \brief start of underlying memory buffer */ - unsigned char *tail; /*!< \brief end of message in buffer */ - unsigned char *data; /*!< \brief start of message in buffer */ unsigned char _data[0]; /*!< \brief optional immediate data array */ };
@@ -179,12 +205,12 @@ static inline int msgb_headroom(const struct msgb *msgb) */ static inline unsigned char *msgb_put(struct msgb *msgb, unsigned int len) { - unsigned char *tmp = msgb->tail; + unsigned char *tmp = msgb->__tail; if (msgb_tailroom(msgb) < (int) len) MSGB_ABORT(msgb, "Not enough tailroom msgb_push (%u < %u)\n", msgb_tailroom(msgb), len); - msgb->tail += len; - msgb->len += len; + msgb->__tail += len; + msgb->__len += len; return tmp; }
@@ -228,12 +254,12 @@ static inline void msgb_put_u32(struct msgb *msgb, uint32_t word) */ static inline unsigned char *msgb_get(struct msgb *msgb, unsigned int len) { - unsigned char *tmp = msgb->data - len; + unsigned char *tmp = msgb->__data - len; if (msgb_length(msgb) < len) MSGB_ABORT(msgb, "msgb too small to get %u (len %u)\n", len, msgb_length(msgb)); - msgb->tail -= len; - msgb->len -= len; + msgb->__tail -= len; + msgb->__len -= len; return tmp; } /*! \brief remove uint8 from end of message @@ -281,9 +307,9 @@ static inline unsigned char *msgb_push(struct msgb *msgb, unsigned int len) if (msgb_headroom(msgb) < (int) len) MSGB_ABORT(msgb, "Not enough headroom msgb_push (%u < %u)\n", msgb_headroom(msgb), len); - msgb->data -= len; - msgb->len += len; - return msgb->data; + msgb->__data -= len; + msgb->__len += len; + return msgb->__data; } /*! \brief remove (pull) a header from the front of the message buffer * \param[in] msgb message buffer @@ -296,8 +322,8 @@ static inline unsigned char *msgb_push(struct msgb *msgb, unsigned int len) */ static inline unsigned char *msgb_pull(struct msgb *msgb, unsigned int len) { - msgb->len -= len; - return msgb->data += len; + msgb->__len -= len; + return msgb->__data += len; }
/*! \brief remove (pull) all headers in front of l3h from the message buffer. @@ -356,8 +382,8 @@ static inline uint32_t msgb_pull_u32(struct msgb *msgb) */ static inline void msgb_reserve(struct msgb *msg, int len) { - msg->data += len; - msg->tail += len; + msg->__data += len; + msg->__tail += len; }
/*! \brief Trim the msgb to a given absolute length @@ -370,8 +396,8 @@ static inline int msgb_trim(struct msgb *msg, int len) if (len > msg->data_len) return -1;
- msg->len = len; - msg->tail = msg->data + len; + msg->__len = len; + msg->__tail = msg->__data + len;
return 0; } diff --git a/src/msgb.c b/src/msgb.c index b2fe1d2..1106824 100644 --- a/src/msgb.c +++ b/src/msgb.c @@ -55,11 +55,11 @@ struct msgb *msgb_alloc(uint16_t size, const char *name) return NULL; }
- msg->data_len = size; - msg->len = 0; - msg->data = msg->_data; - msg->head = msg->_data; - msg->tail = msg->_data; + msg->__data_len = size; + msg->__len = 0; + msg->__data = msg->_data; + msg->__head = msg->_data; + msg->__tail = msg->_data;
return msg; } @@ -113,10 +113,10 @@ struct msgb *msgb_dequeue(struct llist_head *queue) */ void msgb_reset(struct msgb *msg) { - msg->len = 0; - msg->data = msg->_data; - msg->head = msg->_data; - msg->tail = msg->_data; + msg->__len = 0; + msg->__data = msg->_data; + msg->__head = msg->_data; + msg->__tail = msg->_data;
msg->trx = NULL; msg->lchan = NULL; @@ -133,7 +133,7 @@ void msgb_reset(struct msgb *msg) */ uint8_t *msgb_data(const struct msgb *msg) { - return msg->data; + return msg->__data; }
/*! \brief get length of message buffer diff --git a/tests/gsm0808/gsm0808_test.c b/tests/gsm0808/gsm0808_test.c index 7e5e97b..1ce6ef9 100644 --- a/tests/gsm0808/gsm0808_test.c +++ b/tests/gsm0808/gsm0808_test.c @@ -108,8 +108,7 @@ static void test_create_cipher_complete() msgb_free(msg);
/* with l3 data but short */ - l3->len -= 1; - l3->tail -= 1; + msgb_trim(l3, l3->len - 1); msg = gsm0808_create_cipher_complete(l3, 4); VERIFY(msg, res2, ARRAY_SIZE(res2)); msgb_free(msg);
On 04.03.2014 13:44, Jacob Erlbeck wrote:
diff --git a/Doxyfile.core.in b/Doxyfile.core.in index 71843cc..eecd123 100644 --- a/Doxyfile.core.in +++ b/Doxyfile.core.in @@ -1447,7 +1447,7 @@ INCLUDE_FILE_PATTERNS = # undefined via #undef or recursively expanded use the := operator # instead of the = operator.
-PREDEFINED = +PREDEFINED = DOXYGEN
# If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then # this tag can be used to specify a list of macro names that should be expanded.
Don't merge this hunk, please. (It's already removed in the branch)
Jacob
-
Holger Hans Peter Freyther -
Jacob Erlbeck