the armv5 can do 32bit/16bit reads only from the aligned address use tlv.h macro to copy data to local variable
Signed-off-by: Nicolas J. Bouliane nicolas.bouliane@nutaq.com --- src/common/rsl.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/common/rsl.c b/src/common/rsl.c index 9a50a33..5a1b867 100644 --- a/src/common/rsl.c +++ b/src/common/rsl.c @@ -1089,7 +1089,6 @@ static int rsl_tx_ipac_XXcx_ack(struct gsm_lchan *lchan, int inc_pt2, { struct msgb *msg; uint8_t chan_nr = gsm_lchan2chan_nr(lchan); - uint32_t *att_ip; const char *name; struct in_addr ia;
@@ -1116,8 +1115,7 @@ static int rsl_tx_ipac_XXcx_ack(struct gsm_lchan *lchan, int inc_pt2,
/* locally bound IP */ msgb_v_put(msg, RSL_IE_IPAC_LOCAL_IP); - att_ip = (uint32_t *) msgb_put(msg, sizeof(uint32_t)); - *att_ip = htonl(lchan->abis_ip.bound_ip); + msgb_put_u32(msg, lchan->abis_ip.bound_ip);
/* locally bound port */ msgb_tv16_put(msg, RSL_IE_IPAC_LOCAL_PORT, @@ -1199,11 +1197,9 @@ static int tx_ipac_XXcx_nack(struct gsm_lchan *lchan, uint8_t cause, return -ENOMEM;
if (inc_ipport) { - uint32_t *att_ip; /* remote IP */ msgb_v_put(msg, RSL_IE_IPAC_REMOTE_IP); - att_ip = (uint32_t *) msgb_put(msg, sizeof(uint32_t)); - *att_ip = htonl(lchan->abis_ip.connect_ip); + msgb_put_u32(msg, lchan->abis_ip.connect_ip);
/* remote port */ msgb_tv16_put(msg, RSL_IE_IPAC_REMOTE_PORT, @@ -1248,8 +1244,8 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) struct gsm_lchan *lchan = msg->lchan; struct gsm_bts_role_bts *btsb = bts_role_bts(msg->lchan->ts->trx->bts); const uint8_t *payload_type, *speech_mode, *payload_type2; - const uint32_t *connect_ip; - const uint16_t *connect_port; + uint32_t connect_ip = 0; + uint16_t connect_port = 0; int rc, inc_ip_port = 0, port; char *name;
@@ -1267,8 +1263,14 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) speech_mode = TLVP_VAL(&tp, RSL_IE_IPAC_SPEECH_MODE); payload_type = TLVP_VAL(&tp, RSL_IE_IPAC_RTP_PAYLOAD); payload_type2 = TLVP_VAL(&tp, RSL_IE_IPAC_RTP_PAYLOAD2); - connect_ip = (uint32_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_IP); - connect_port = (uint16_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_PORT); + + if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_IP)) { + connect_ip = tlvp_val32_unal(&tp, RSL_IE_IPAC_REMOTE_IP); + } + + if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_PORT)) { + connect_port = tlvp_val16_unal(&tp, RSL_IE_IPAC_REMOTE_PORT); + }
if (dch->c.msg_type == RSL_MT_IPAC_CRCX && connect_ip && connect_port) inc_ip_port = 1; @@ -1350,14 +1352,14 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg)
/* Special rule: If connect_ip == 0.0.0.0, use RSL IP * address */ - if (*connect_ip == 0) { + if (connect_ip == 0) { struct ipabis_link *link = lchan->ts->trx->rsl_link; ia.s_addr = htonl(link->ip); } else - ia.s_addr = *connect_ip; + ia.s_addr = connect_ip; rc = osmo_rtp_socket_connect(lchan->abis_ip.rtp_socket, - inet_ntoa(ia), ntohs(*connect_port)); + inet_ntoa(ia), ntohs(connect_port)); if (rc < 0) { LOGP(DRSL, LOGL_ERROR, "%s Failed to connect RTP/RTCP sockets\n", @@ -1370,7 +1372,7 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) } /* save IP address and port number */ lchan->abis_ip.connect_ip = ntohl(ia.s_addr); - lchan->abis_ip.connect_port = ntohs(*connect_port); + lchan->abis_ip.connect_port = ntohs(connect_port);
} else { /* FIXME: discard all codec frames */
Hi Nicolas,
thanks for your patch.
On Thu, Apr 18, 2013 at 08:25:31AM -0400, Nicolas J. Bouliane wrote:
- connect_ip = (uint32_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_IP);
- connect_port = (uint16_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_PORT);
- if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_IP)) {
connect_ip = tlvp_val32_unal(&tp, RSL_IE_IPAC_REMOTE_IP);- }
- if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_PORT)) {
connect_port = tlvp_val16_unal(&tp, RSL_IE_IPAC_REMOTE_PORT);- }
Several comments for that:
1) we unconditionally assume that the IEs are present in the original code. This is most likely due to the fact that they are specified to be there. If they don't exist, we fail miserably. With your new code, we still fail in strange ways. Best would be to print a LOGL_WARN or ERROR message about missing information elements.
2) kernel coding style (which we use universally in our projects) says: If there's only a single statement after an if statement, we don't use curly braces.
I'd appreciate if you could submit a new patch (replacing the original patch, not an incremental one) to that regard. Thanks.
Regards, Harald
the armv5 can do 32bit/16bit reads only from the aligned address use tlv.h macro to copy data to local variable
Signed-off-by: Nicolas J. Bouliane nicolas.bouliane@nutaq.com --- src/common/rsl.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/common/rsl.c b/src/common/rsl.c index 9a50a33..f3657a7 100644 --- a/src/common/rsl.c +++ b/src/common/rsl.c @@ -1089,7 +1089,6 @@ static int rsl_tx_ipac_XXcx_ack(struct gsm_lchan *lchan, int inc_pt2, { struct msgb *msg; uint8_t chan_nr = gsm_lchan2chan_nr(lchan); - uint32_t *att_ip; const char *name; struct in_addr ia;
@@ -1116,8 +1115,7 @@ static int rsl_tx_ipac_XXcx_ack(struct gsm_lchan *lchan, int inc_pt2,
/* locally bound IP */ msgb_v_put(msg, RSL_IE_IPAC_LOCAL_IP); - att_ip = (uint32_t *) msgb_put(msg, sizeof(uint32_t)); - *att_ip = htonl(lchan->abis_ip.bound_ip); + msgb_put_u32(msg, lchan->abis_ip.bound_ip);
/* locally bound port */ msgb_tv16_put(msg, RSL_IE_IPAC_LOCAL_PORT, @@ -1199,11 +1197,9 @@ static int tx_ipac_XXcx_nack(struct gsm_lchan *lchan, uint8_t cause, return -ENOMEM;
if (inc_ipport) { - uint32_t *att_ip; /* remote IP */ msgb_v_put(msg, RSL_IE_IPAC_REMOTE_IP); - att_ip = (uint32_t *) msgb_put(msg, sizeof(uint32_t)); - *att_ip = htonl(lchan->abis_ip.connect_ip); + msgb_put_u32(msg, lchan->abis_ip.connect_ip);
/* remote port */ msgb_tv16_put(msg, RSL_IE_IPAC_REMOTE_PORT, @@ -1248,8 +1244,8 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) struct gsm_lchan *lchan = msg->lchan; struct gsm_bts_role_bts *btsb = bts_role_bts(msg->lchan->ts->trx->bts); const uint8_t *payload_type, *speech_mode, *payload_type2; - const uint32_t *connect_ip; - const uint16_t *connect_port; + uint32_t connect_ip = 0; + uint16_t connect_port = 0; int rc, inc_ip_port = 0, port; char *name;
@@ -1267,8 +1263,14 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) speech_mode = TLVP_VAL(&tp, RSL_IE_IPAC_SPEECH_MODE); payload_type = TLVP_VAL(&tp, RSL_IE_IPAC_RTP_PAYLOAD); payload_type2 = TLVP_VAL(&tp, RSL_IE_IPAC_RTP_PAYLOAD2); - connect_ip = (uint32_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_IP); - connect_port = (uint16_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_PORT); + + if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_IP)) + connect_ip = tlvp_val32_unal(&tp, RSL_IE_IPAC_REMOTE_IP); + else LOGP(DRSL, LOGL_NOTICE, "CRCX does not specify a remote IP\n"); + + if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_PORT)) + connect_port = tlvp_val16_unal(&tp, RSL_IE_IPAC_REMOTE_PORT); + else LOGP(DRSL, LOGL_NOTICE, "CRCX does not specify a remote port\n");
if (dch->c.msg_type == RSL_MT_IPAC_CRCX && connect_ip && connect_port) inc_ip_port = 1; @@ -1350,14 +1352,14 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg)
/* Special rule: If connect_ip == 0.0.0.0, use RSL IP * address */ - if (*connect_ip == 0) { + if (connect_ip == 0) { struct ipabis_link *link = lchan->ts->trx->rsl_link; ia.s_addr = htonl(link->ip); } else - ia.s_addr = *connect_ip; + ia.s_addr = connect_ip; rc = osmo_rtp_socket_connect(lchan->abis_ip.rtp_socket, - inet_ntoa(ia), ntohs(*connect_port)); + inet_ntoa(ia), ntohs(connect_port)); if (rc < 0) { LOGP(DRSL, LOGL_ERROR, "%s Failed to connect RTP/RTCP sockets\n", @@ -1370,7 +1372,7 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) } /* save IP address and port number */ lchan->abis_ip.connect_ip = ntohl(ia.s_addr); - lchan->abis_ip.connect_port = ntohs(*connect_port); + lchan->abis_ip.connect_port = ntohs(connect_port);
} else { /* FIXME: discard all codec frames */
the armv5 can do 32bit/16bit reads only from the aligned address use tlv.h macro to copy data to local variable
Signed-off-by: Nicolas J. Bouliane nicolas.bouliane@nutaq.com --- src/common/rsl.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/src/common/rsl.c b/src/common/rsl.c index 9a50a33..4f28cb2 100644 --- a/src/common/rsl.c +++ b/src/common/rsl.c @@ -1089,7 +1089,6 @@ static int rsl_tx_ipac_XXcx_ack(struct gsm_lchan *lchan, int inc_pt2, { struct msgb *msg; uint8_t chan_nr = gsm_lchan2chan_nr(lchan); - uint32_t *att_ip; const char *name; struct in_addr ia;
@@ -1116,8 +1115,7 @@ static int rsl_tx_ipac_XXcx_ack(struct gsm_lchan *lchan, int inc_pt2,
/* locally bound IP */ msgb_v_put(msg, RSL_IE_IPAC_LOCAL_IP); - att_ip = (uint32_t *) msgb_put(msg, sizeof(uint32_t)); - *att_ip = htonl(lchan->abis_ip.bound_ip); + msgb_put_u32(msg, lchan->abis_ip.bound_ip);
/* locally bound port */ msgb_tv16_put(msg, RSL_IE_IPAC_LOCAL_PORT, @@ -1199,11 +1197,9 @@ static int tx_ipac_XXcx_nack(struct gsm_lchan *lchan, uint8_t cause, return -ENOMEM;
if (inc_ipport) { - uint32_t *att_ip; /* remote IP */ msgb_v_put(msg, RSL_IE_IPAC_REMOTE_IP); - att_ip = (uint32_t *) msgb_put(msg, sizeof(uint32_t)); - *att_ip = htonl(lchan->abis_ip.connect_ip); + msgb_put_u32(msg, lchan->abis_ip.connect_ip);
/* remote port */ msgb_tv16_put(msg, RSL_IE_IPAC_REMOTE_PORT, @@ -1248,8 +1244,8 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) struct gsm_lchan *lchan = msg->lchan; struct gsm_bts_role_bts *btsb = bts_role_bts(msg->lchan->ts->trx->bts); const uint8_t *payload_type, *speech_mode, *payload_type2; - const uint32_t *connect_ip; - const uint16_t *connect_port; + uint32_t connect_ip = 0; + uint16_t connect_port = 0; int rc, inc_ip_port = 0, port; char *name;
@@ -1267,8 +1263,16 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) speech_mode = TLVP_VAL(&tp, RSL_IE_IPAC_SPEECH_MODE); payload_type = TLVP_VAL(&tp, RSL_IE_IPAC_RTP_PAYLOAD); payload_type2 = TLVP_VAL(&tp, RSL_IE_IPAC_RTP_PAYLOAD2); - connect_ip = (uint32_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_IP); - connect_port = (uint16_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_PORT); + + if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_IP)) + connect_ip = tlvp_val32_unal(&tp, RSL_IE_IPAC_REMOTE_IP); + else + LOGP(DRSL, LOGL_NOTICE, "CRCX does not specify a remote IP\n"); + + if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_PORT)) + connect_port = tlvp_val16_unal(&tp, RSL_IE_IPAC_REMOTE_PORT); + else + LOGP(DRSL, LOGL_NOTICE, "CRCX does not specify a remote port\n");
if (dch->c.msg_type == RSL_MT_IPAC_CRCX && connect_ip && connect_port) inc_ip_port = 1; @@ -1350,14 +1354,14 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg)
/* Special rule: If connect_ip == 0.0.0.0, use RSL IP * address */ - if (*connect_ip == 0) { + if (connect_ip == 0) { struct ipabis_link *link = lchan->ts->trx->rsl_link; ia.s_addr = htonl(link->ip); } else - ia.s_addr = *connect_ip; + ia.s_addr = connect_ip; rc = osmo_rtp_socket_connect(lchan->abis_ip.rtp_socket, - inet_ntoa(ia), ntohs(*connect_port)); + inet_ntoa(ia), ntohs(connect_port)); if (rc < 0) { LOGP(DRSL, LOGL_ERROR, "%s Failed to connect RTP/RTCP sockets\n", @@ -1370,7 +1374,7 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) } /* save IP address and port number */ lchan->abis_ip.connect_ip = ntohl(ia.s_addr); - lchan->abis_ip.connect_port = ntohs(*connect_port); + lchan->abis_ip.connect_port = ntohs(connect_port);
} else { /* FIXME: discard all codec frames */
Dear Nicolas,
I'm not sure what you're doing or where exactly your patch got broken, but both "git am" as well as a manual "patch -p1" completely fail:
patching file src/common/rsl.c Hunk #1 FAILED at 1089. Hunk #2 FAILED at 1116. Hunk #3 FAILED at 1199. Hunk #4 FAILED at 1248. Hunk #5 FAILED at 1267. Hunk #6 FAILED at 1350. Hunk #7 FAILED at 1370. 7 out of 7 hunks FAILED -- saving rejects to file src/common/rsl.c.rej
To me it seems like your file contains four spaces instead of tab indentation that we use. I suggest sending mails by "git send-email" or buy git format-patch + attaching them to your mail.
Regards, Harald
On Sun, Apr 21, 2013 at 05:41:22PM +0200, Harald Welte wrote:
Yes,
we already had this on IRC. thunderbird is replacing tabs with spaces. Maybe we should have a procmail file that rejects inline patches sent by thunderbird. :}
To me it seems like your file contains four spaces instead of tab indentation that we use. I suggest sending mails by "git send-email" or buy git format-patch + attaching them to your mail.
From: "Nicolas J. Bouliane" nicolas.bouliane@nutaq.com
the armv5 can do 32bit/16bit reads only from the aligned address use tlv.h macro to copy data to local variable
Signed-off-by: Nicolas J. Bouliane nicolas.bouliane@nutaq.com --- src/common/rsl.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/src/common/rsl.c b/src/common/rsl.c index 9a50a33..4f28cb2 100644 --- a/src/common/rsl.c +++ b/src/common/rsl.c @@ -1089,7 +1089,6 @@ static int rsl_tx_ipac_XXcx_ack(struct gsm_lchan *lchan, int inc_pt2, { struct msgb *msg; uint8_t chan_nr = gsm_lchan2chan_nr(lchan); - uint32_t *att_ip; const char *name; struct in_addr ia;
@@ -1116,8 +1115,7 @@ static int rsl_tx_ipac_XXcx_ack(struct gsm_lchan *lchan, int inc_pt2,
/* locally bound IP */ msgb_v_put(msg, RSL_IE_IPAC_LOCAL_IP); - att_ip = (uint32_t *) msgb_put(msg, sizeof(uint32_t)); - *att_ip = htonl(lchan->abis_ip.bound_ip); + msgb_put_u32(msg, lchan->abis_ip.bound_ip);
/* locally bound port */ msgb_tv16_put(msg, RSL_IE_IPAC_LOCAL_PORT, @@ -1199,11 +1197,9 @@ static int tx_ipac_XXcx_nack(struct gsm_lchan *lchan, uint8_t cause, return -ENOMEM;
if (inc_ipport) { - uint32_t *att_ip; /* remote IP */ msgb_v_put(msg, RSL_IE_IPAC_REMOTE_IP); - att_ip = (uint32_t *) msgb_put(msg, sizeof(uint32_t)); - *att_ip = htonl(lchan->abis_ip.connect_ip); + msgb_put_u32(msg, lchan->abis_ip.connect_ip);
/* remote port */ msgb_tv16_put(msg, RSL_IE_IPAC_REMOTE_PORT, @@ -1248,8 +1244,8 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) struct gsm_lchan *lchan = msg->lchan; struct gsm_bts_role_bts *btsb = bts_role_bts(msg->lchan->ts->trx->bts); const uint8_t *payload_type, *speech_mode, *payload_type2; - const uint32_t *connect_ip; - const uint16_t *connect_port; + uint32_t connect_ip = 0; + uint16_t connect_port = 0; int rc, inc_ip_port = 0, port; char *name;
@@ -1267,8 +1263,16 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) speech_mode = TLVP_VAL(&tp, RSL_IE_IPAC_SPEECH_MODE); payload_type = TLVP_VAL(&tp, RSL_IE_IPAC_RTP_PAYLOAD); payload_type2 = TLVP_VAL(&tp, RSL_IE_IPAC_RTP_PAYLOAD2); - connect_ip = (uint32_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_IP); - connect_port = (uint16_t *) TLVP_VAL(&tp, RSL_IE_IPAC_REMOTE_PORT); + + if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_IP)) + connect_ip = tlvp_val32_unal(&tp, RSL_IE_IPAC_REMOTE_IP); + else + LOGP(DRSL, LOGL_NOTICE, "CRCX does not specify a remote IP\n"); + + if (TLVP_PRESENT(&tp, RSL_IE_IPAC_REMOTE_PORT)) + connect_port = tlvp_val16_unal(&tp, RSL_IE_IPAC_REMOTE_PORT); + else + LOGP(DRSL, LOGL_NOTICE, "CRCX does not specify a remote port\n");
if (dch->c.msg_type == RSL_MT_IPAC_CRCX && connect_ip && connect_port) inc_ip_port = 1; @@ -1350,14 +1354,14 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg)
/* Special rule: If connect_ip == 0.0.0.0, use RSL IP * address */ - if (*connect_ip == 0) { + if (connect_ip == 0) { struct ipabis_link *link = lchan->ts->trx->rsl_link; ia.s_addr = htonl(link->ip); } else - ia.s_addr = *connect_ip; + ia.s_addr = connect_ip; rc = osmo_rtp_socket_connect(lchan->abis_ip.rtp_socket, - inet_ntoa(ia), ntohs(*connect_port)); + inet_ntoa(ia), ntohs(connect_port)); if (rc < 0) { LOGP(DRSL, LOGL_ERROR, "%s Failed to connect RTP/RTCP sockets\n", @@ -1370,7 +1374,7 @@ static int rsl_rx_ipac_XXcx(struct msgb *msg) } /* save IP address and port number */ lchan->abis_ip.connect_ip = ntohl(ia.s_addr); - lchan->abis_ip.connect_port = ntohs(*connect_port); + lchan->abis_ip.connect_port = ntohs(connect_port);
} else { /* FIXME: discard all codec frames */
On Mon, Apr 22, 2013 at 07:45:13AM -0400, Nicolas J. Bouliane wrote:
From: "Nicolas J. Bouliane" nicolas.bouliane@nutaq.com
the armv5 can do 32bit/16bit reads only from the aligned address use tlv.h macro to copy data to local variable
thanks, I have applied it now and it should tickle to master as part of my other changes.
holger
-
Harald Welte -
Holger Hans Peter Freyther -
Nicolas J. Bouliane