The old code overwrites correct values with wrong ones. Send a nokia business card
over sms to a kannel instance running smpp to see what I mean.
There are no real efficiency implications to my patch. The int variable udh_offset is
initialized to 0, so if esm_class doesn't set UDHI, the line after the conditional
does The Right Thing.
As for not using the #defines... Guilty as charged. I'll resubmit with the comments
cleaned up and using the #define.
Thanks!
Dave WHITE
CONNECT AUSTRIA
-- original message --
Subject: Re: smsc_smpp.c -- some minor fixes
From: Alexander Malysh <[EMAIL PROTECTED]>
Date: Tue, 11 Feb 2003 18:57:22 +0100
Hi all,
-1 from me... see comments...
Am Dienstag, 11. Februar 2003 15:55 schrieb Dave White:
> Here's a patch that fixes two deficiencies with Kannel's SMPP
> implementation.
>
> *) UDH for MO messages is not supported in the unpatched driver.
>
> This patch will correctly deal with the most common (SMPP 3.3-style)
> technique for sending MO's with UDH, namely esm_class |= 64 with the UDH
> placed in the short_message field.
>
> *) Coding for some message types is broken.
>
> I changed smsc_smpp.c to rely on the (dcs|fields)to(fields|dcs)
> functions in sms.c, and to only mess with the values if it does a
> transformation to UCS2 or iso-8859-1.
>
> I have some minor qualms about the patch.
>
> There was
> pdu->u.deliver_sm.short_message = NULL;
> in pdu_to_msg in smsc_smpp.c which I honestly don't understand.
>
> Also, I'm not entirely clear on how memory is freed in Kannel from the
> Msg structure. Is there something like a destructor for the generated
> datastructures Kannel uses? Could someone give me a pointer (groan)
> where I should look to understand this?
>
> So there is a small chance that it may leak a little memory. I suspect
> the usual segfault on a "free the dangling pointer" would have bit me by
> now if there were something wrong, but I don't want to rule that out
> until a few more eyes have a look at it.
>
> Testing on my system(s) "works" -- i.e. no observed crashes or memory
> leak warnings, and correct handling of every kind of SMS data I could
> throw at it.
>
> Have fun,
>
> David WHITE
> CONNECT AUSTRIA
>
> -- PATCH --
>
> ===================================================================
> RCS file: /home/cvs/gateway/gw/smsc/smsc_smpp.c,v
> retrieving revision 1.22
> diff -u -r1.22 smsc_smpp.c
> --- gw/smsc/smsc_smpp.c 2 Jan 2003 14:43:00 -0000 1.22
> +++ gw/smsc/smsc_smpp.c 11 Feb 2003 14:48:37 -0000
> @@ -233,7 +233,8 @@
>
> static Msg *pdu_to_msg(SMPP *smpp, SMPP_PDU *pdu)
> {
> - Msg *msg;
> + Msg *msg;
> + int udh_offset = 0;
>
> gw_assert(pdu->type == deliver_sm);
>
> @@ -242,10 +243,19 @@
> pdu->u.deliver_sm.source_addr = NULL;
> msg->sms.receiver = pdu->u.deliver_sm.destination_addr;
> pdu->u.deliver_sm.destination_addr = NULL;
> - msg->sms.msgdata = pdu->u.deliver_sm.short_message;
> - pdu->u.deliver_sm.short_message = NULL;
Set pdu->u.deliver_sm.short_message to NULL is OK, so we does't need to copy/alloc.
octstr
(this is more efficiently).
> + /* msg->sms.msgdata = pdu->u.deliver_sm.short_message; - done
> later -- DAVE
> + pdu->u.deliver_sm.short_message = NULL; */
> dcs_to_fields(&msg, pdu->u.deliver_sm.data_coding);
>
> + /* DAVE -- actually handle UDH */
> + if (pdu->u.deliver_sm.esm_class & 0x40) {
Why you do not use defines ? (e.g. pdu->u.deliver_sm.esm_class &
ESM_CLASS_SUBMIT_UDH_INDICATOR)
All (I believe) values are defined in smpp_pdu.h
> + udh_offset = octstr_get_char(pdu->u.deliver_sm.short_message,0)+1;
> + msg->sms.udhdata =
> octstr_copy(pdu->u.deliver_sm.short_message,0,udh_offset);
if we set pdu->u.deliver_sm.short_message=NULL above , this will not work ;(
It should be:
udh_offset = octstr_get_char(msg->sms.msgdata,0)+1;
if (udh_offset > octstr_len(msg->sms.msgdata)) /* just sanity check */
goto error;
msg->sms.udhdata = octstr_copy(msg->sms.msgdata,0,udh_offset);
octstr_delete(msg->sms.msgdata, 0, udh_offset);
This is (imho) more efficiently , because not every sms will have udh set ...
> + }
> + info(0, "SMPP: pdu->u.deliver_sm.short_message = %x, udh_offset =
> %d",pdu->u.deliver_sm.short_message,udh_offset);
> + msg->sms.msgdata =
> octstr_copy(pdu->u.deliver_sm.short_message,udh_offset,octstr_len(pdu->u.de
>liver_sm.short_message)-udh_offset); +
> +
> /* handle default data coding */
> switch (pdu->u.deliver_sm.data_coding) {
> case 0x00: /* default SMSC alphabet */
> @@ -257,11 +267,11 @@
> if (charset_convert(msg->sms.msgdata,
> octstr_get_cstr(smpp->alt_charset), "ISO-8859-1") != 0)
> error(0, "Failed to convert msgdata from charset
> <%s> to <%s>, will leave as is.",
> octstr_get_cstr(smpp->alt_charset),
> "ISO-8859-1");- msg->sms.coding = DC_7BIT;
> + msg->sms.coding = DC_7BIT;
+1 from me...
>
> } else { /* assume GSM 03.38 7-bit alphabet */
> charset_gsm_to_latin1(msg->sms.msgdata);
> - msg->sms.coding = DC_7BIT;
> + /* DAVE -- Don't do this, it was set OK by
> dcs_to_fields msg->sms.coding = DC_7BIT;*/
Why not set ? safe is safe ;)
> }
> break;
> case 0x01: /* ASCII or IA5 - not sure if I need to do anything */
> @@ -269,7 +279,7 @@
> case 0x04: /* 8 bit binary - do nothing */
> break;
> case 0x03: /* ISO-8859-1 - do nothing */
> - msg->sms.coding = DC_8BIT; break;
> + /* DAVE -- Don't do this, it was set OK by
> dcs_to_fields msg->sms.coding = DC_8BIT;*/ break;
Why not set ? safe is safe ;)
> case 0x05: /* JIS - what do I do with that ? */
> break;
> case 0x06: /* Cyrllic - iso-8859-5, I'll convert to unicode */
> @@ -288,7 +298,7 @@
> * you implement them if you feel like it
> */
> default:
> - msg->sms.coding = DC_7BIT;
> + /* DAVE -- Don't do this, it was set OK by dcs_to_fields
> msg->sms.coding = DC_7BIT; */
> }
> msg->sms.pid = pdu->u.deliver_sm.protocol_id;
--
Best regards / Mit besten Grüßen aus Köln
Dipl.-Ing.
Alexander Malysh
___________________________________________
Centrium GmbH
Ehrenstraße 2
50672 Köln
Fon: +49 (0221) 277 49 240
Fax: +49 (0221) 277 49 109
email: [EMAIL PROTECTED]
web: www.centrium.de
msn: [EMAIL PROTECTED]
