openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
Patch Set 3: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/3470 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 Gerrit-PatchSet: 3 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Pablo Neira AyusoGerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pablo Neira Ayuso Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
[MERGED] openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
Harald Welte has submitted this change and it was merged. Change subject: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV handling .. libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV handling submit_to_sms() now handles two TLVs, so find_tlv() is suboptiomal and it can be removed, since it would result in two passes on the TLV list. Use new smpp34_tlv_for_each() helper to iterate over the list of TLVs that is available since I446929feed049d0411e1629ca263e2bc41f714cc. Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 --- M openbsc/src/libmsc/smpp_openbsc.c 1 file changed, 41 insertions(+), 31 deletions(-) Approvals: Harald Welte: Looks good to me, approved Jenkins Builder: Verified diff --git a/openbsc/src/libmsc/smpp_openbsc.c b/openbsc/src/libmsc/smpp_openbsc.c index 85de040..7e23abd 100644 --- a/openbsc/src/libmsc/smpp_openbsc.c +++ b/openbsc/src/libmsc/smpp_openbsc.c @@ -72,26 +72,31 @@ return subscr; } -/*! \brief find a TLV with given tag in list of libsmpp34 TLVs */ -static struct tlv_t *find_tlv(struct tlv_t *head, uint16_t tag) +static int smpp34_submit_tlv_msg_payload(const struct tlv_t *t, +const struct submit_sm_t *submit, +const uint8_t **sms_msg, +unsigned int *sms_msg_len) { - struct tlv_t *t; - - for (t = head; t != NULL; t = t->next) { - if (t->tag == tag) - return t; + if (submit->sm_length) { + LOGP(DLSMS, LOGL_ERROR, +"SMPP cannot have payload in TLV _and_ in the header\n"); + return -1; } - return NULL; + *sms_msg = t->value.octet; + *sms_msg_len = t->length; + + return 0; } /*! \brief convert from submit_sm_t to gsm_sms */ static int submit_to_sms(struct gsm_sms **psms, struct gsm_network *net, const struct submit_sm_t *submit) { + const uint8_t *sms_msg = NULL; struct gsm_subscriber *dest; + uint16_t msg_ref = 0; struct gsm_sms *sms; struct tlv_t *t; - const uint8_t *sms_msg; unsigned int sms_msg_len; int mode; @@ -105,31 +110,40 @@ return ESME_RINVDSTADR; } - t = find_tlv(submit->tlv, TLVID_message_payload); - if (t) { - if (submit->sm_length) { - /* ERROR: we cannot have both! */ - LOGP(DLSMS, LOGL_ERROR, "SMPP Cannot have payload in " - "TLV _and_ in the header\n"); - subscr_put(dest); - return ESME_ROPTPARNOTALLWD; + smpp34_tlv_for_each(t, submit->tlv) { + switch (t->tag) { + case TLVID_message_payload: + if (smpp34_submit_tlv_msg_payload(t, submit, _msg, + _msg_len) < 0) { + subscr_put(dest); + return ESME_ROPTPARNOTALLWD; + } + break; + case TLVID_user_message_reference: + msg_ref = ntohs(t->value.val16); + break; + default: + break; } - sms_msg = t->value.octet; - sms_msg_len = t->length; - } else if (submit->sm_length > 0 && submit->sm_length < 255) { - sms_msg = submit->short_message; - sms_msg_len = submit->sm_length; - } else { - LOGP(DLSMS, LOGL_ERROR, - "SMPP neither message payload nor valid sm_length.\n"); - subscr_put(dest); - return ESME_RINVPARLEN; + } + + if (!sms_msg) { + if (submit->sm_length > 0 && submit->sm_length < 255) { + sms_msg = submit->short_message; + sms_msg_len = submit->sm_length; + } else { + LOGP(DLSMS, LOGL_ERROR, +"SMPP neither message payload nor valid sm_length.\n"); + subscr_put(dest); + return ESME_RINVPARLEN; + } } sms = sms_alloc(); sms->source = SMS_SOURCE_SMPP; sms->smpp.sequence_nr = submit->sequence_number; sms->status_rep_req = submit->registered_delivery; + sms->msg_ref = msg_ref; /* fill in the destination address */ sms->receiver = dest; @@ -203,10 +217,6 @@ memcpy(sms->user_data, sms_msg, sms_msg_len); sms->user_data_len = sms_msg_len; } - - t = find_tlv(submit->tlv, TLVID_user_message_reference); - if (t) - sms->msg_ref =
[PATCH] openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/3470 to look at the new patch set (#3). libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV handling submit_to_sms() now handles two TLVs, so find_tlv() is suboptiomal and it can be removed, since it would result in two passes on the TLV list. Use new smpp34_tlv_for_each() helper to iterate over the list of TLVs that is available since I446929feed049d0411e1629ca263e2bc41f714cc. Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 --- M openbsc/src/libmsc/smpp_openbsc.c 1 file changed, 41 insertions(+), 31 deletions(-) git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/70/3470/3 diff --git a/openbsc/src/libmsc/smpp_openbsc.c b/openbsc/src/libmsc/smpp_openbsc.c index 85de040..7e23abd 100644 --- a/openbsc/src/libmsc/smpp_openbsc.c +++ b/openbsc/src/libmsc/smpp_openbsc.c @@ -72,26 +72,31 @@ return subscr; } -/*! \brief find a TLV with given tag in list of libsmpp34 TLVs */ -static struct tlv_t *find_tlv(struct tlv_t *head, uint16_t tag) +static int smpp34_submit_tlv_msg_payload(const struct tlv_t *t, +const struct submit_sm_t *submit, +const uint8_t **sms_msg, +unsigned int *sms_msg_len) { - struct tlv_t *t; - - for (t = head; t != NULL; t = t->next) { - if (t->tag == tag) - return t; + if (submit->sm_length) { + LOGP(DLSMS, LOGL_ERROR, +"SMPP cannot have payload in TLV _and_ in the header\n"); + return -1; } - return NULL; + *sms_msg = t->value.octet; + *sms_msg_len = t->length; + + return 0; } /*! \brief convert from submit_sm_t to gsm_sms */ static int submit_to_sms(struct gsm_sms **psms, struct gsm_network *net, const struct submit_sm_t *submit) { + const uint8_t *sms_msg = NULL; struct gsm_subscriber *dest; + uint16_t msg_ref = 0; struct gsm_sms *sms; struct tlv_t *t; - const uint8_t *sms_msg; unsigned int sms_msg_len; int mode; @@ -105,31 +110,40 @@ return ESME_RINVDSTADR; } - t = find_tlv(submit->tlv, TLVID_message_payload); - if (t) { - if (submit->sm_length) { - /* ERROR: we cannot have both! */ - LOGP(DLSMS, LOGL_ERROR, "SMPP Cannot have payload in " - "TLV _and_ in the header\n"); - subscr_put(dest); - return ESME_ROPTPARNOTALLWD; + smpp34_tlv_for_each(t, submit->tlv) { + switch (t->tag) { + case TLVID_message_payload: + if (smpp34_submit_tlv_msg_payload(t, submit, _msg, + _msg_len) < 0) { + subscr_put(dest); + return ESME_ROPTPARNOTALLWD; + } + break; + case TLVID_user_message_reference: + msg_ref = ntohs(t->value.val16); + break; + default: + break; } - sms_msg = t->value.octet; - sms_msg_len = t->length; - } else if (submit->sm_length > 0 && submit->sm_length < 255) { - sms_msg = submit->short_message; - sms_msg_len = submit->sm_length; - } else { - LOGP(DLSMS, LOGL_ERROR, - "SMPP neither message payload nor valid sm_length.\n"); - subscr_put(dest); - return ESME_RINVPARLEN; + } + + if (!sms_msg) { + if (submit->sm_length > 0 && submit->sm_length < 255) { + sms_msg = submit->short_message; + sms_msg_len = submit->sm_length; + } else { + LOGP(DLSMS, LOGL_ERROR, +"SMPP neither message payload nor valid sm_length.\n"); + subscr_put(dest); + return ESME_RINVPARLEN; + } } sms = sms_alloc(); sms->source = SMS_SOURCE_SMPP; sms->smpp.sequence_nr = submit->sequence_number; sms->status_rep_req = submit->registered_delivery; + sms->msg_ref = msg_ref; /* fill in the destination address */ sms->receiver = dest; @@ -203,10 +217,6 @@ memcpy(sms->user_data, sms_msg, sms_msg_len); sms->user_data_len = sms_msg_len; } - - t = find_tlv(submit->tlv, TLVID_user_message_reference); - if (t) - sms->msg_ref = ntohs(t->value.val16); *psms = sms; return ESME_ROK; -- To view, visit
openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
Patch Set 2: I never looked at that library so far, and I cannot give +2 on it anyway, sorry for that. Regarding your "retrigger by pushing again", I give you a tip for that: You can re-trigger the jenkins job which was launched by Gerrit and it if succeeds, it will automatically give Verify+1 to the patch. You can find the link to the last job in one of the old comments in this patch, which is posted by "Jenkins Builder". Follow the link to jenkins and press "Retrigger" button which, if you have enough permissions, should appear on the left side of the web page. -- To view, visit https://gerrit.osmocom.org/3470 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 Gerrit-PatchSet: 2 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Pablo Neira AyusoGerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pablo Neira Ayuso Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
Patch Set 2: Oh, I forgot. You can probably speed up the process if you give me +X to my libsmpp34 patches (if you're familiar with that code). Thanks! -- To view, visit https://gerrit.osmocom.org/3470 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 Gerrit-PatchSet: 2 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Pablo Neira AyusoGerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pablo Neira Ayuso Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
Patch Set 2: OK, will include this info once patches for libsmpp34 get in. So this also helps re-kick Jenkins building. -- To view, visit https://gerrit.osmocom.org/3470 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 Gerrit-PatchSet: 2 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Pablo Neira AyusoGerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pablo Neira Ayuso Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
Patch Set 2: You could provide info about the dependency in the form "Requires libsmpp34 Change-Id XYZ" in the commit message, or at least here as a comment. This way is easier to track what needs to be merged, or which version of the library is used. -- To view, visit https://gerrit.osmocom.org/3470 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 Gerrit-PatchSet: 2 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Pablo Neira AyusoGerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pablo Neira Ayuso Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
Patch Set 2: Build breaks because this patch depends on a patch in libsmpp34. -- To view, visit https://gerrit.osmocom.org/3470 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 Gerrit-PatchSet: 2 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Pablo Neira AyusoGerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pablo Neira Ayuso Gerrit-HasComments: No
[PATCH] openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV handling submit_to_sms() now handles two TLVs, so find_tlv() is suboptiomal and it can be removed, since it would result in two passes on the TLV list. Use new smpp34_tlv_for_each() helper to iterate over the list of TLVs. Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 --- M openbsc/src/libmsc/smpp_openbsc.c 1 file changed, 41 insertions(+), 31 deletions(-) git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/70/3470/2 diff --git a/openbsc/src/libmsc/smpp_openbsc.c b/openbsc/src/libmsc/smpp_openbsc.c index 85de040..7e23abd 100644 --- a/openbsc/src/libmsc/smpp_openbsc.c +++ b/openbsc/src/libmsc/smpp_openbsc.c @@ -72,26 +72,31 @@ return subscr; } -/*! \brief find a TLV with given tag in list of libsmpp34 TLVs */ -static struct tlv_t *find_tlv(struct tlv_t *head, uint16_t tag) +static int smpp34_submit_tlv_msg_payload(const struct tlv_t *t, +const struct submit_sm_t *submit, +const uint8_t **sms_msg, +unsigned int *sms_msg_len) { - struct tlv_t *t; - - for (t = head; t != NULL; t = t->next) { - if (t->tag == tag) - return t; + if (submit->sm_length) { + LOGP(DLSMS, LOGL_ERROR, +"SMPP cannot have payload in TLV _and_ in the header\n"); + return -1; } - return NULL; + *sms_msg = t->value.octet; + *sms_msg_len = t->length; + + return 0; } /*! \brief convert from submit_sm_t to gsm_sms */ static int submit_to_sms(struct gsm_sms **psms, struct gsm_network *net, const struct submit_sm_t *submit) { + const uint8_t *sms_msg = NULL; struct gsm_subscriber *dest; + uint16_t msg_ref = 0; struct gsm_sms *sms; struct tlv_t *t; - const uint8_t *sms_msg; unsigned int sms_msg_len; int mode; @@ -105,31 +110,40 @@ return ESME_RINVDSTADR; } - t = find_tlv(submit->tlv, TLVID_message_payload); - if (t) { - if (submit->sm_length) { - /* ERROR: we cannot have both! */ - LOGP(DLSMS, LOGL_ERROR, "SMPP Cannot have payload in " - "TLV _and_ in the header\n"); - subscr_put(dest); - return ESME_ROPTPARNOTALLWD; + smpp34_tlv_for_each(t, submit->tlv) { + switch (t->tag) { + case TLVID_message_payload: + if (smpp34_submit_tlv_msg_payload(t, submit, _msg, + _msg_len) < 0) { + subscr_put(dest); + return ESME_ROPTPARNOTALLWD; + } + break; + case TLVID_user_message_reference: + msg_ref = ntohs(t->value.val16); + break; + default: + break; } - sms_msg = t->value.octet; - sms_msg_len = t->length; - } else if (submit->sm_length > 0 && submit->sm_length < 255) { - sms_msg = submit->short_message; - sms_msg_len = submit->sm_length; - } else { - LOGP(DLSMS, LOGL_ERROR, - "SMPP neither message payload nor valid sm_length.\n"); - subscr_put(dest); - return ESME_RINVPARLEN; + } + + if (!sms_msg) { + if (submit->sm_length > 0 && submit->sm_length < 255) { + sms_msg = submit->short_message; + sms_msg_len = submit->sm_length; + } else { + LOGP(DLSMS, LOGL_ERROR, +"SMPP neither message payload nor valid sm_length.\n"); + subscr_put(dest); + return ESME_RINVPARLEN; + } } sms = sms_alloc(); sms->source = SMS_SOURCE_SMPP; sms->smpp.sequence_nr = submit->sequence_number; sms->status_rep_req = submit->registered_delivery; + sms->msg_ref = msg_ref; /* fill in the destination address */ sms->receiver = dest; @@ -203,10 +217,6 @@ memcpy(sms->user_data, sms_msg, sms_msg_len); sms->user_data_len = sms_msg_len; } - - t = find_tlv(submit->tlv, TLVID_user_message_reference); - if (t) - sms->msg_ref = ntohs(t->value.val16); *psms = sms; return ESME_ROK; -- To view, visit https://gerrit.osmocom.org/3470 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 Gerrit-PatchSet: 2 Gerrit-Project:
[PATCH] openbsc[master]: libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV ha...
Review at https://gerrit.osmocom.org/3470 libmsc: use smpp34_tlv_for_each() to avoid suboptimal TLV handling submit_to_sms() now handles two TLVs, so find_tlv() is suboptiomal and it can be removed, since it would result in two passes on the TLV list. Use new smpp34_tlv_for_each() helper to iterate over the list of TLVs. Change-Id: I53a65164a6cc4abc6bf57d9a8dc275cf21c90222 --- M openbsc/src/libmsc/smpp_openbsc.c 1 file changed, 41 insertions(+), 31 deletions(-) git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/70/3470/1 diff --git a/openbsc/src/libmsc/smpp_openbsc.c b/openbsc/src/libmsc/smpp_openbsc.c index 85de040..b2944a9 100644 --- a/openbsc/src/libmsc/smpp_openbsc.c +++ b/openbsc/src/libmsc/smpp_openbsc.c @@ -72,26 +72,31 @@ return subscr; } -/*! \brief find a TLV with given tag in list of libsmpp34 TLVs */ -static struct tlv_t *find_tlv(struct tlv_t *head, uint16_t tag) +static int smpp34_submit_tlv_msg_payload(const struct tlv_t *t, +const struct submit_sm_t *submit, +const uint8_t **sms_msg, +unsigned int *sms_msg_len) { - struct tlv_t *t; - - for (t = head; t != NULL; t = t->next) { - if (t->tag == tag) - return t; + if (submit->sm_length) { + LOGP(DLSMS, LOGL_ERROR, +"SMPP cannot have payload in TLV _and_ in the header\n"); + return -1; } - return NULL; + *sms_msg = t->value.octet; + *sms_msg_len = t->length; + + return 0; } /*! \brief convert from submit_sm_t to gsm_sms */ static int submit_to_sms(struct gsm_sms **psms, struct gsm_network *net, const struct submit_sm_t *submit) { + const uint8_t *sms_msg = NULL; struct gsm_subscriber *dest; + uint8_t msg_ref = 0; struct gsm_sms *sms; struct tlv_t *t; - const uint8_t *sms_msg; unsigned int sms_msg_len; int mode; @@ -105,31 +110,40 @@ return ESME_RINVDSTADR; } - t = find_tlv(submit->tlv, TLVID_message_payload); - if (t) { - if (submit->sm_length) { - /* ERROR: we cannot have both! */ - LOGP(DLSMS, LOGL_ERROR, "SMPP Cannot have payload in " - "TLV _and_ in the header\n"); - subscr_put(dest); - return ESME_ROPTPARNOTALLWD; + smpp34_tlv_for_each(t, submit->tlv) { + switch (t->tag) { + case TLVID_message_payload: + if (smpp34_submit_tlv_msg_payload(t, submit, _msg, + _msg_len) < 0) { + subscr_put(dest); + return ESME_ROPTPARNOTALLWD; + } + break; + case TLVID_user_message_reference: + msg_ref = ntohs(t->value.val16); + break; + default: + break; } - sms_msg = t->value.octet; - sms_msg_len = t->length; - } else if (submit->sm_length > 0 && submit->sm_length < 255) { - sms_msg = submit->short_message; - sms_msg_len = submit->sm_length; - } else { - LOGP(DLSMS, LOGL_ERROR, - "SMPP neither message payload nor valid sm_length.\n"); - subscr_put(dest); - return ESME_RINVPARLEN; + } + + if (!sms_msg) { + if (submit->sm_length > 0 && submit->sm_length < 255) { + sms_msg = submit->short_message; + sms_msg_len = submit->sm_length; + } else { + LOGP(DLSMS, LOGL_ERROR, +"SMPP neither message payload nor valid sm_length.\n"); + subscr_put(dest); + return ESME_RINVPARLEN; + } } sms = sms_alloc(); sms->source = SMS_SOURCE_SMPP; sms->smpp.sequence_nr = submit->sequence_number; sms->status_rep_req = submit->registered_delivery; + sms->msg_ref = msg_ref; /* fill in the destination address */ sms->receiver = dest; @@ -203,10 +217,6 @@ memcpy(sms->user_data, sms_msg, sms_msg_len); sms->user_data_len = sms_msg_len; } - - t = find_tlv(submit->tlv, TLVID_user_message_reference); - if (t) - sms->msg_ref = ntohs(t->value.val16); *psms = sms; return ESME_ROK; -- To view, visit https://gerrit.osmocom.org/3470 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: