> 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.
will be tested ;)
> 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.
You have misundestanding me. Why always copy octstr from pdu into msgdata ?
If udhi is not set we can just point msgdata to pdu message and set pdu
message to NULL, so pdu message will not be destroyed and we do not need to
alloc./copy octstr. (This is what smpp driver do already).
> 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]
-------------------------------------------------------
--
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]
