Hi,
I just looked at a trace and saw that the RP-MessageReference we assign is hardcoded to 42. It should be a sequence number going from 0 to 255. It is somehow idiotic as there can only be one SMS per direction and there is the transaction identifier as well.
I would propose a patch that does: Change gsm411_send_sms to ask the subscriber connection/transaction code for a message reference. For added bonus also "put" the number so we could verify to not re-use the number too early.
anyones wants to come up with a fix?
holger
On Wed, Feb 05, 2014 at 09:27:06AM +0100, Holger Hans Peter Freyther wrote:
anyones wants to come up with a fix?
No one? Fairwaves? Somebody at least want to try the below code and see if SMS can still be sent?
diff --git a/openbsc/include/openbsc/gsm_data.h b/openbsc/include/openbsc/gsm_data.h index 41fe328..404dfe4 100644 --- a/openbsc/include/openbsc/gsm_data.h +++ b/openbsc/include/openbsc/gsm_data.h @@ -106,6 +106,8 @@ struct gsm_subscriber_connection {
/* LU expiration handling */ uint8_t expire_timer_stopped; + /* SMS helpers for libmsc */ + uint8_t next_rp_ref;
/* * Operations that have a state and might be pending diff --git a/openbsc/src/libmsc/gsm_04_11.c b/openbsc/src/libmsc/gsm_04_11.c index 97a67ee..433951a 100644 --- a/openbsc/src/libmsc/gsm_04_11.c +++ b/openbsc/src/libmsc/gsm_04_11.c @@ -64,6 +64,18 @@ extern int smpp_try_deliver(struct gsm_sms *sms, void *tall_gsms_ctx; static uint32_t new_callref = 0x40000001;
+uint8_t sms_next_rp_msg_ref(struct gsm_subscriber_connection *conn) +{ + const uint8_t rp_msg_ref = conn->next_rp_ref; + /* + * This should wrap as the valid range is 0 to 255. We only + * transfer one SMS at a time so we don't need to check if + * the id has been already assigned. + */ + conn->next_rp_ref += 1; + + return rp_msg_ref; +}
struct gsm_sms *sms_alloc(void) { @@ -450,7 +462,7 @@ static int gsm411_rp_sendmsg(struct gsm411_smr_inst *inst, struct msgb *msg, rp = (struct gsm411_rp_hdr *)msgb_push(msg, sizeof(*rp)); rp->len = len + 2; rp->msg_type = rp_msg_type; - rp->msg_ref = rp_msg_ref; /* FIXME: Choose randomly */ + rp->msg_ref = rp_msg_ref;
return gsm411_smr_send(inst, rl_msg_type, msg); } @@ -836,7 +848,7 @@ int gsm411_send_sms(struct gsm_subscriber_connection *conn, struct gsm_sms *sms) struct msgb *msg = gsm411_msgb_alloc(); struct gsm_trans *trans; uint8_t *data, *rp_ud_len; - uint8_t msg_ref = 42; + uint8_t msg_ref = sms_next_rp_msg_ref(conn); int transaction_id; int rc;
On Sat, Feb 8, 2014 at 6:18 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Wed, Feb 05, 2014 at 09:27:06AM +0100, Holger Hans Peter Freyther wrote:
anyones wants to come up with a fix?
No one? Fairwaves? Somebody at least want to try the below code and see if SMS can still be sent?
I think it would be better if you just write a unit test for this issue.
On Sat, Feb 08, 2014 at 09:04:21PM +0400, Alexander Chemeris wrote:
Hi,
No one? Fairwaves? Somebody at least want to try the below code and see if SMS can still be sent?
I think it would be better if you just write a unit test for this issue.
I just remembered that you said you work on SMS issues and I wanted to give you an opportunity to contribute to OpenBSC. Sure you are not required to contribute. ;)
holger
Hi Holger,
On Sun, Feb 9, 2014 at 10:51 AM, Holger Hans Peter Freyther holger@freyther.de wrote:
On Sat, Feb 08, 2014 at 09:04:21PM +0400, Alexander Chemeris wrote:
No one? Fairwaves? Somebody at least want to try the below code and see if SMS can still be sent?
I think it would be better if you just write a unit test for this issue.
I just remembered that you said you work on SMS issues and I wanted to give you an opportunity to contribute to OpenBSC. Sure you are not required to contribute. ;)
May be it's just me, but I find it impolite to ask someone to test a patch, just because you don't bother to test it yourself. Moreover when there it no a unit-test for the change - at least to make sure that the next_rp_ref doesn't overflow.
We would be happy to test the patch in production environment, once it's at least initially tested and merged to mainline. So far we're busy with our own set of issues to be fixed, which are more severe than this one. Patches to be contributed soon.
On Sun, Feb 09, 2014 at 01:35:46PM +0400, Alexander Chemeris wrote:
Alexander,
May be it's just me, but I find it impolite to ask someone to test a patch, just because you don't bother to test it yourself. Moreover when there it no a unit-test for the change - at least to make sure that the next_rp_ref doesn't overflow.
next_rp_ref and the message reference are of the same range. So it is intended to overlflow and I skipped the % UINT8_MAX.
We would be happy to test the patch in production environment, once it's at least initially tested and merged to mainline. So far we're busy with our own set of issues to be fixed, which are more severe than this one. Patches to be contributed soon.
In German we say "lieber den spatz in der hand, als die taube auf dem dach". We prepfer a little spaze over promises. I have seen your marketing about the contributions of Fairwaves, I have seen the promises of fixes coming. I just thought I could motivate you guys to actually fix something easy. I was wrong and there is no point in replying to this email. ;)
holger
-
Alexander Chemeris -
Holger Hans Peter Freyther