From 3b0991e80fca70a784ab5c58b67cd22d86bc221d Mon Sep 17 00:00:00 2001 From: Pau Espin Pedrol Date: Wed, 31 Aug 2022 17:28:36 +0200 Subject: [PATCH] osmux: Allocate struct osmux_out_handle through API Until now, the osmux_out_handle was allocated by the client, and passed to the API to initialize it. This makes it really hard to improve the implementation without breaking the ABI. Let's break the ABI now one last time (hopefully) by allocating the struct through an API. With only this change, the already built users (osmo-mgw, openbsc) can still work fine, since there's no change on the struct osmux_out_handle. However, they will somehow break next time the struct is changed until they are ported to the same API (easy to do). Related: OS#5987 Change-Id: Ie8df581f375c9a183a7af60b431561bda82f6e34 --- TODO-RELEASE | 1 + examples/osmux-test-output.c | 12 +-- include/osmocom/netif/osmux.h | 7 +- src/osmux.c | 53 ++++++++++++++ tests/jibuf/jibuf_tool.c | 13 ++-- tests/osmux/osmux_test.c | 14 ++-- tests/osmux/osmux_test2.c | 133 ++++++++++++++++++++-------------- 7 files changed, 161 insertions(+), 72 deletions(-) diff --git a/TODO-RELEASE b/TODO-RELEASE index d0852fc..52a8c49 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -7,3 +7,4 @@ # If any interfaces have been added since the last public release: c:r:a + 1. # If any interfaces have been removed or changed since the last public release: c:r:0. #library what description / commit summary line +libosmo-netif osmux new osmux_xfrm_output_* APIs diff --git a/examples/osmux-test-output.c b/examples/osmux-test-output.c index d39277b..3f0e5a2 100644 --- a/examples/osmux-test-output.c +++ b/examples/osmux-test-output.c @@ -42,7 +42,7 @@ static struct osmo_rtp_handle *rtp; * This is the output handle for osmux, it stores last RTP sequence and * timestamp that has been used. There should be one per circuit ID. */ -static struct osmux_out_handle h_output; +static struct osmux_out_handle *h_output; static int fd; @@ -107,7 +107,7 @@ int read_cb(struct osmo_dgram *conn) msg->len, buf); while((osmuxh = osmux_xfrm_output_pull(msg)) != NULL) - osmux_xfrm_output_sched(&h_output, osmuxh); + osmux_xfrm_output_sched(h_output, osmuxh); return 0; } @@ -117,7 +117,7 @@ void sighandler(int foo) LOGP(DOSMUX_TEST, LOGL_NOTICE, "closing OSMUX.\n"); osmo_dgram_close(conn); osmo_dgram_destroy(conn); - osmux_xfrm_output_flush(&h_output); + talloc_free(h_output); osmo_rtp_handle_free(rtp); amr_close(); exit(EXIT_SUCCESS); @@ -155,8 +155,10 @@ int main(int argc, char *argv[]) /* * initialize OSMUX handlers. */ - osmux_xfrm_output_init2(&h_output, random(), 98); - osmux_xfrm_output_set_tx_cb(&h_output, tx_cb, NULL); + h_output = osmux_xfrm_output_alloc(NULL); + osmux_xfrm_output_set_rtp_ssrc(h_output, random()); + osmux_xfrm_output_set_rtp_pl_type(h_output, 98); + osmux_xfrm_output_set_tx_cb(h_output, tx_cb, NULL); /* * initialize datagram server. */ diff --git a/include/osmocom/netif/osmux.h b/include/osmocom/netif/osmux.h index 8d39344..8742797 100644 --- a/include/osmocom/netif/osmux.h +++ b/include/osmocom/netif/osmux.h @@ -107,8 +107,11 @@ void osmux_xfrm_input_close_circuit(struct osmux_in_handle *h, int ccid); int osmux_xfrm_input(struct osmux_in_handle *h, struct msgb *msg, int ccid); void osmux_xfrm_input_deliver(struct osmux_in_handle *h); -void osmux_xfrm_output_init(struct osmux_out_handle *h, uint32_t rtp_ssrc) OSMO_DEPRECATED("Use osmux_xfrm_output_init2() instead"); -void osmux_xfrm_output_init2(struct osmux_out_handle *h, uint32_t rtp_ssrc, uint8_t rtp_payload_type); +struct osmux_out_handle *osmux_xfrm_output_alloc(void *ctx); +void osmux_xfrm_output_init(struct osmux_out_handle *h, uint32_t rtp_ssrc) OSMO_DEPRECATED("Use osmux_xfrm_output_alloc() and osmux_xfrm_output_set_rtp_*() instead"); +void osmux_xfrm_output_init2(struct osmux_out_handle *h, uint32_t rtp_ssrc, uint8_t rtp_payload_type) OSMO_DEPRECATED("Use osmux_xfrm_output_alloc() and osmux_xfrm_output_set_rtp_*() instead"); +void osmux_xfrm_output_set_rtp_ssrc(struct osmux_out_handle *h, uint32_t rtp_ssrc); +void osmux_xfrm_output_set_rtp_pl_type(struct osmux_out_handle *h, uint32_t rtp_payload_type); void osmux_xfrm_output_set_tx_cb(struct osmux_out_handle *h, void (*tx_cb)(struct msgb *msg, void *data), void *data); int osmux_xfrm_output_sched(struct osmux_out_handle *h, struct osmux_hdr *osmuxh); void osmux_xfrm_output_flush(struct osmux_out_handle *h); diff --git a/src/osmux.c b/src/osmux.c index bf03cc5..63342fa 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -864,6 +864,40 @@ struct osmux_tx_handle { void *data; }; +static int osmux_xfrm_output_talloc_destructor(struct osmux_out_handle *h) +{ + osmux_xfrm_output_flush(h); + return 0; +} + +/*! \brief Allocate a new osmux out handle + * \param[in] ctx talloc context to use when allocating the returned struct + * \return Allocated osmux out handle + * + * This object contains configuration and state to handle a specific CID in + * incoming network Osmux messages, repackaging the frames for that CID as RTP + * packets and pushing them up the protocol stack. + * Returned pointer can be freed with regular talloc_free, queue will be flushed + * and all internal data will be freed. */ +struct osmux_out_handle *osmux_xfrm_output_alloc(void *ctx) +{ + struct osmux_out_handle *h; + + h = talloc_zero(ctx, struct osmux_out_handle); + OSMO_ASSERT(h); + + h->rtp_seq = (uint16_t)random(); + h->rtp_timestamp = (uint32_t)random(); + h->rtp_ssrc = (uint32_t)random(); + h->rtp_payload_type = 98; + INIT_LLIST_HEAD(&h->list); + osmo_timer_setup(&h->timer, osmux_xfrm_output_trigger, h); + + talloc_set_destructor(h, osmux_xfrm_output_talloc_destructor); + return h; +} + +/* DEPRECATED: Use osmux_xfrm_output_alloc() and osmux_xfrm_output_set_rtp_*() instead */ void osmux_xfrm_output_init2(struct osmux_out_handle *h, uint32_t rtp_ssrc, uint8_t rtp_payload_type) { memset(h, 0, sizeof(*h)); @@ -875,6 +909,7 @@ void osmux_xfrm_output_init2(struct osmux_out_handle *h, uint32_t rtp_ssrc, uint osmo_timer_setup(&h->timer, osmux_xfrm_output_trigger, h); } +/* DEPRECATED: Use osmux_xfrm_output_alloc() and osmux_xfrm_output_set_rtp_*() instead */ void osmux_xfrm_output_init(struct osmux_out_handle *h, uint32_t rtp_ssrc) { /* backward compatibility with old users, where 98 was harcoded in osmux_rebuild_rtp() */ @@ -897,6 +932,24 @@ void osmux_xfrm_output_set_tx_cb(struct osmux_out_handle *h, h->data = data; } +/*! \brief Set SSRC of generated RTP packets from Osmux frames + * \param[in] h the osmux out handle handling a specific CID + * \param[in] rtp_ssrc the RTP SSRC to set + */ +void osmux_xfrm_output_set_rtp_ssrc(struct osmux_out_handle *h, uint32_t rtp_ssrc) +{ + h->rtp_ssrc = rtp_ssrc; +} + +/*! \brief Set Payload Type of generated RTP packets from Osmux frames + * \param[in] h the osmux out handle handling a specific CID + * \param[in] rtp_payload_type the RTP Payload Type to set + */ +void osmux_xfrm_output_set_rtp_pl_type(struct osmux_out_handle *h, uint32_t rtp_payload_type) +{ + h->rtp_payload_type = rtp_payload_type; +} + #define SNPRINTF_BUFFER_SIZE(ret, remain, offset) \ if (ret < 0) \ ret = 0; \ diff --git a/tests/jibuf/jibuf_tool.c b/tests/jibuf/jibuf_tool.c index 969ef8b..b69686a 100644 --- a/tests/jibuf/jibuf_tool.c +++ b/tests/jibuf/jibuf_tool.c @@ -113,7 +113,7 @@ static uint32_t packets_too_much_jitter; /* Used for test pcap: */ static struct osmo_pcap osmo_pcap; static bool pcap_finished; -static struct osmux_out_handle pcap_osmux_h; +static struct osmux_out_handle *pcap_osmux_h; /* ----------------------------- */ static void sigalarm_handler(int foo) @@ -438,7 +438,7 @@ int pcap_read_osmux(struct msgb *msg) /* This code below belongs to the osmux receiver */ while((osmuxh = osmux_xfrm_output_pull(msg)) != NULL) - osmux_xfrm_output_sched(&pcap_osmux_h, osmuxh); + osmux_xfrm_output_sched(pcap_osmux_h, osmuxh); msgb_free(msg); return 0; } @@ -517,8 +517,10 @@ void pcap_test() { osmo_pcap.timer.cb = pcap_pkt_timer_cb; if(opt_osmux) { - osmux_xfrm_output_init2(&pcap_osmux_h, 0, 98); - osmux_xfrm_output_set_tx_cb(&pcap_osmux_h, glue_cb, NULL); + pcap_osmux_h = osmux_xfrm_output_alloc(NULL); + osmux_xfrm_output_set_rtp_ssrc(pcap_osmux_h, 0); + osmux_xfrm_output_set_rtp_pl_type(pcap_osmux_h, 98); + osmux_xfrm_output_set_tx_cb(pcap_osmux_h, glue_cb, NULL); } jb = osmo_jibuf_alloc(NULL); @@ -532,11 +534,12 @@ void pcap_test() { while(!pcap_finished || !osmo_jibuf_empty(jb)) { if (pcap_finished && opt_osmux) /* Flushing once should be enough */ - osmux_xfrm_output_flush(&pcap_osmux_h); + osmux_xfrm_output_flush(pcap_osmux_h); osmo_select_main(0); } osmo_jibuf_delete(jb); + talloc_free(pcap_osmux_h); pcap_test_check(); } diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c index f0edeab..54e9368 100644 --- a/tests/osmux/osmux_test.c +++ b/tests/osmux/osmux_test.c @@ -123,7 +123,7 @@ static void tx_cb(struct msgb *msg, void *data) msgb_free(msg); } -static struct osmux_out_handle h_output; +static struct osmux_out_handle *h_output; static void osmux_deliver(struct msgb *batch_msg, void *data) { @@ -137,7 +137,7 @@ static void osmux_deliver(struct msgb *batch_msg, void *data) * in a list. Then, reconstruct transmission timing. */ while((osmuxh = osmux_xfrm_output_pull(batch_msg)) != NULL) - osmux_xfrm_output_sched(&h_output, osmuxh); + osmux_xfrm_output_sched(h_output, osmuxh); msgb_free(batch_msg); } @@ -298,11 +298,13 @@ int main(void) log_set_print_category_hex(osmo_stderr_target, 0); log_set_use_color(osmo_stderr_target, 0); - osmux_xfrm_output_init2(&h_output, 0x7000000, 98); - osmux_xfrm_output_set_tx_cb(&h_output, tx_cb, NULL); + h_output = osmux_xfrm_output_alloc(NULL); + osmux_xfrm_output_set_rtp_ssrc(h_output, 0x7000000); + osmux_xfrm_output_set_rtp_pl_type(h_output, 98); + osmux_xfrm_output_set_tx_cb(h_output, tx_cb, NULL); /* These fields are set using random() */ - h_output.rtp_seq = 9158; - h_output.rtp_timestamp = 1681692777; + h_output->rtp_seq = 9158; + h_output->rtp_timestamp = 1681692777; /* If the test takes longer than 10 seconds, abort it */ alarm(10); diff --git a/tests/osmux/osmux_test2.c b/tests/osmux/osmux_test2.c index 092f4bf..7a66920 100644 --- a/tests/osmux/osmux_test2.c +++ b/tests/osmux/osmux_test2.c @@ -157,20 +157,23 @@ static void tx_cb(struct msgb *msg, void *data) static void test_output_consecutive(void) { - struct osmux_out_handle h_output; + struct osmux_out_handle *h_output; printf("===test_output_consecutive===\n"); clock_override_enable(true); clock_override_set(0, 0); osmux_init(32); - osmux_xfrm_output_init2(&h_output, 0x7000000, 98); - h_output.rtp_seq = (uint16_t)50; - h_output.rtp_timestamp = (uint32_t)500; - osmux_xfrm_output_set_tx_cb(&h_output, tx_cb, &h_output); + + h_output = osmux_xfrm_output_alloc(NULL); + osmux_xfrm_output_set_rtp_ssrc(h_output, 0x7000000); + osmux_xfrm_output_set_rtp_pl_type(h_output, 98); + osmux_xfrm_output_set_tx_cb(h_output, tx_cb, h_output); + h_output->rtp_seq = (uint16_t)50; + h_output->rtp_timestamp = (uint32_t)500; /* First osmux frame at t=0 */ - PULL_NEXT(&h_output); + PULL_NEXT(h_output); clock_debug("first dequed before first select"); osmo_select_main(0); @@ -193,11 +196,11 @@ static void test_output_consecutive(void) clock_override_add(0, TIME_RTP_PKT_MS*1000); clock_debug("sixth select, sixth dequed"); osmo_select_main(0); - OSMO_ASSERT(llist_empty(&h_output.list)); + OSMO_ASSERT(llist_empty(&h_output->list)); /* Second osmux frame at t=80 */ clock_debug("send second osmux frame"); - PULL_NEXT(&h_output); + PULL_NEXT(h_output); clock_debug("first dequed before first select"); osmo_select_main(0); @@ -208,33 +211,38 @@ static void test_output_consecutive(void) clock_override_add(0, 4*TIME_RTP_PKT_MS*1000); clock_debug("third select, four packet should be dequeued"); osmo_select_main(0); - OSMO_ASSERT(llist_empty(&h_output.list)); - OSMO_ASSERT(!osmo_timer_pending(&h_output.timer)); + OSMO_ASSERT(llist_empty(&h_output->list)); + OSMO_ASSERT(!osmo_timer_pending(&h_output->timer)); clock_debug("calling flush on empty list, should do nothing"); - osmux_xfrm_output_flush(&h_output); - OSMO_ASSERT(llist_empty(&h_output.list)); - OSMO_ASSERT(!osmo_timer_pending(&h_output.timer)); + osmux_xfrm_output_flush(h_output); + OSMO_ASSERT(llist_empty(&h_output->list)); + OSMO_ASSERT(!osmo_timer_pending(&h_output->timer)); + + talloc_free(h_output); } static void test_output_interleaved(void) { - struct osmux_out_handle h_output; + struct osmux_out_handle *h_output; printf("===test_output_interleaved===\n"); clock_override_enable(true); clock_override_set(0, 0); osmux_init(32); - osmux_xfrm_output_init2(&h_output, 0x7000000, 98); - h_output.rtp_seq = (uint16_t)50; - h_output.rtp_timestamp = (uint32_t)500; - osmux_xfrm_output_set_tx_cb(&h_output, tx_cb, &h_output); + + h_output = osmux_xfrm_output_alloc(NULL); + osmux_xfrm_output_set_rtp_ssrc(h_output, 0x7000000); + osmux_xfrm_output_set_rtp_pl_type(h_output, 98); + osmux_xfrm_output_set_tx_cb(h_output, tx_cb, h_output); + h_output->rtp_seq = (uint16_t)50; + h_output->rtp_timestamp = (uint32_t)500; /* First osmux frame at t=0, but it actually arrives late due to jitter, so 2nd frame is going to arrive before the 1st one is completelly scheduled */ - PULL_NEXT(&h_output); + PULL_NEXT(h_output); clock_override_add(0, 2*TIME_RTP_PKT_MS*1000); clock_debug("select, 3 dequed, 3 still queued"); @@ -242,68 +250,78 @@ static void test_output_interleaved(void) /* Second osmux frame at t=0 */ clock_debug("next frame arrives, 3 pending rtp packets are dequeued and first of new osmux frame too"); - PULL_NEXT(&h_output); + PULL_NEXT(h_output); osmo_select_main(0); - OSMO_ASSERT(llist_count(&h_output.list) == 5); + OSMO_ASSERT(llist_count(&h_output->list) == 5); clock_override_add(0, 5*TIME_RTP_PKT_MS*1000); clock_debug("calling select, then all should be out"); osmo_select_main(0); - OSMO_ASSERT(llist_empty(&h_output.list)); - OSMO_ASSERT(!osmo_timer_pending(&h_output.timer)); + OSMO_ASSERT(llist_empty(&h_output->list)); + OSMO_ASSERT(!osmo_timer_pending(&h_output->timer)); + + talloc_free(h_output); } static void test_output_2together(void) { - struct osmux_out_handle h_output; + struct osmux_out_handle *h_output; printf("===test_output_2together===\n"); clock_override_enable(true); clock_override_set(0, 0); osmux_init(32); - osmux_xfrm_output_init2(&h_output, 0x7000000, 98); - h_output.rtp_seq = (uint16_t)50; - h_output.rtp_timestamp = (uint32_t)500; - osmux_xfrm_output_set_tx_cb(&h_output, tx_cb, &h_output); + + h_output = osmux_xfrm_output_alloc(NULL); + osmux_xfrm_output_set_rtp_ssrc(h_output, 0x7000000); + osmux_xfrm_output_set_rtp_pl_type(h_output, 98); + osmux_xfrm_output_set_tx_cb(h_output, tx_cb, h_output); + h_output->rtp_seq = (uint16_t)50; + h_output->rtp_timestamp = (uint32_t)500; /* First osmux frame at t=0, but it actually arrives late due to jitter, so we receive both at the same time. */ - PULL_NEXT(&h_output); + PULL_NEXT(h_output); clock_debug("calling select in between 2 osmux recv"); osmo_select_main(0); - PULL_NEXT(&h_output); + PULL_NEXT(h_output); clock_debug("calling select after receiving 2nd osmux. Dequeue 1st osmux frame and 1st rtp from 2nd osmux frame."); osmo_select_main(0); - OSMO_ASSERT(llist_count(&h_output.list) == 5); + OSMO_ASSERT(llist_count(&h_output->list) == 5); clock_override_add(0, 5*TIME_RTP_PKT_MS*1000); clock_debug("select, all 5 remaining should be out"); osmo_select_main(0); - OSMO_ASSERT(llist_empty(&h_output.list)); - OSMO_ASSERT(!osmo_timer_pending(&h_output.timer)); + OSMO_ASSERT(llist_empty(&h_output->list)); + OSMO_ASSERT(!osmo_timer_pending(&h_output->timer)); + + talloc_free(h_output); } /* FIXME: this test shows generated rtp stream doesn't have osmux lost frames into account! */ static void test_output_frame_lost(void) { - struct osmux_out_handle h_output; + struct osmux_out_handle *h_output; printf("===test_output_frame_lost===\n"); clock_override_enable(true); clock_override_set(0, 0); osmux_init(32); - osmux_xfrm_output_init2(&h_output, 0x7000000, 98); - h_output.rtp_seq = (uint16_t)50; - h_output.rtp_timestamp = (uint32_t)500; - osmux_xfrm_output_set_tx_cb(&h_output, tx_cb, &h_output); + + h_output = osmux_xfrm_output_alloc(NULL); + osmux_xfrm_output_set_rtp_ssrc(h_output, 0x7000000); + osmux_xfrm_output_set_rtp_pl_type(h_output, 98); + osmux_xfrm_output_set_tx_cb(h_output, tx_cb, h_output); + h_output->rtp_seq = (uint16_t)50; + h_output->rtp_timestamp = (uint32_t)500; clock_debug("first osmux frame"); - PULL_NEXT(&h_output); + PULL_NEXT(h_output); clock_override_add(0, 5*TIME_RTP_PKT_MS*1000); osmo_select_main(0); @@ -312,42 +330,49 @@ static void test_output_frame_lost(void) clock_override_add(0, 6*TIME_RTP_PKT_MS*1000); clock_debug("3rd osmux frame arrives"); - PULL_NEXT(&h_output); + PULL_NEXT(h_output); clock_override_add(0, 5*TIME_RTP_PKT_MS*1000); osmo_select_main(0); - OSMO_ASSERT(llist_empty(&h_output.list)); - OSMO_ASSERT(!osmo_timer_pending(&h_output.timer)); + OSMO_ASSERT(llist_empty(&h_output->list)); + OSMO_ASSERT(!osmo_timer_pending(&h_output->timer)); + + talloc_free(h_output); } static void test_output_flush(void) { - struct osmux_out_handle h_output; + struct osmux_out_handle *h_output; printf("===test_output_flush===\n"); clock_override_enable(true); clock_override_set(0, 0); osmux_init(32); - osmux_xfrm_output_init2(&h_output, 0x7000000, 98); - h_output.rtp_seq = (uint16_t)50; - h_output.rtp_timestamp = (uint32_t)500; - osmux_xfrm_output_set_tx_cb(&h_output, tx_cb, &h_output); + + h_output = osmux_xfrm_output_alloc(NULL); + osmux_xfrm_output_set_rtp_ssrc(h_output, 0x7000000); + osmux_xfrm_output_set_rtp_pl_type(h_output, 98); + osmux_xfrm_output_set_tx_cb(h_output, tx_cb, h_output); + h_output->rtp_seq = (uint16_t)50; + h_output->rtp_timestamp = (uint32_t)500; clock_debug("first osmux frame"); - PULL_NEXT(&h_output); + PULL_NEXT(h_output); clock_override_add(0, 2*TIME_RTP_PKT_MS*1000); osmo_select_main(0); clock_debug("2nd osmux frame arrives"); - PULL_NEXT(&h_output); + PULL_NEXT(h_output); clock_debug("flushing, all packet should be transmitted immediately"); - OSMO_ASSERT(llist_count(&h_output.list) == 9); - OSMO_ASSERT(osmo_timer_pending(&h_output.timer)); - osmux_xfrm_output_flush(&h_output); - OSMO_ASSERT(llist_empty(&h_output.list)); - OSMO_ASSERT(!osmo_timer_pending(&h_output.timer)); + OSMO_ASSERT(llist_count(&h_output->list) == 9); + OSMO_ASSERT(osmo_timer_pending(&h_output->timer)); + osmux_xfrm_output_flush(h_output); + OSMO_ASSERT(llist_empty(&h_output->list)); + OSMO_ASSERT(!osmo_timer_pending(&h_output->timer)); + + talloc_free(h_output); } int main(int argc, char **argv)