RE: CMTI-patch?

Sorry for my last email - I did not see this one before I sent it.

Here are my comments for the patch you submitted
- You are not following the Kannel standards as for debugging messages - take a look at other examples for debug() calls in the code.
- please dont use strcpy() or other c-string handling functions, as those are easy to break - especially as you are not doing any sanity checks in the input. use the octstr class functions instead.
- the whole think of treating +CMTI as a completly different case then the other notifictions is not a good idea IMHO, the recursive call is not nice, but cannot be avoided.

all in all I think the code is not as clean as it should be, and I would really like it to be well integrated with the sim-buffering functions which implement much of the needed functionality.

my suggestion would be to break the sim-buffering functions into parts and use those for the relevant calls. it will also be eaiser to integrate cross memory type queries that way.

-1 for this patch.

--
Oded Arbel
m-Wise mobile solutions
[email protected]

+972-9-9581711 (116)
+972-67-340014

::..
If glory comes after death, I'm not in a hurry. (Latin proverb)


> -----Original Message-----
> From: Rene Kluwen / Chimit Software Solutions 
> [mailto:[email protected]]
> Sent: Wednesday, September 18, 2002 11:18 PM
> To: [email protected]
> Subject: CMTI-patch?
> 
> 
> Hello devel,
> 
>   I heard the preffered method is submitting patches is cvs diff -u.
>   Even though I have a little bit of experience with both SMS as well
>   as WAP (stack) development, I am new to Kannel.
>   I must say, the code is very well modularized and also well
>   documented (a thank-you to the authors ;]).
> 
>   Below is a patch to have kannel respond to +CMTI, sms incoming
>   notifications.
>   For me, this was neccesary because my phone (Siemens SL45i) does not
>   send full pdu's to the TE, but rather sends indications at last.
> 
>   See below the patch. My question about it is:
>   Am I not breaking things with this? I realize that
>   at2_wait_modem_command is called cross-recursively with this patch.
>   Error status is not checked... Because in case of an error, there is
>   little that I can do about it (except maybe a log message).
> 
>   Also, maybe the return value of at2_wait_modem_command can be
>   erroneous. But I am still not that much into the code to see if that
>   is correct. Feedback is appreciated. For me, at least - so far it
>   works well.
> 
>   To test this patch, set init-string = "AT+CNMI=1,1,0,2,1" (the
>   second parameter has to be 1).
>   Also, sim-buffering is set to false at best, since both methods
>   could intervene each other.
> 
>   Any feedback?
> 
> ===================================================================
> RCS file: /home/cvs/gateway/gw/smsc/smsc_at2.c,v
> retrieving revision 1.5
> diff -u -r1.5 smsc_at2.c
> --- gw/smsc/smsc_at2.c  6 Sep 2002 09:35:09 -0000       1.5
> +++ gw/smsc/smsc_at2.c  18 Sep 2002 21:04:35 -0000
> @@ -493,6 +493,11 @@
>      int len;
>      int cmgr_flag = 0;
>  
> +    // Chimit: we use cmd to request a message from store at 
> an incoming SMS indication
> +    //         cmti_pos is going to hold the memory position 
> at which the message came in
> +    char cmd[20];
> +    char cmti_pos[20];
> +
>      time(&end_time);
>      if (timeout == 0)
>          timeout = 3;
> @@ -534,6 +539,24 @@
>                  ret = 1;
>                  goto end;
>              }
> +           // Chimit: These 18 lines handle incoming message 
> indication for phones that do not
> +           //         support full PDU notification, like 
> Siemens SL45i.
> +           //         Todo: Some phones have messages come 
> in at another message store than
> +           //               the preferred message store for 
> reading (ericsson, as I may recall?).
> +           //               This is not handled in this code.
> +           if (octstr_search(line, octstr_imm("+CMTI:"), 0) != -1) {
> +               // we received an incoming message indication
> +               debug("smsc_at2.c", 0, "+CMTI incoming SMS 
> indication: %s", octstr_get_cstr(line));
> +               // determine which memory position
> +               strcpy(cmti_pos, octstr_get_cstr(line) + 12);
> +               // now read the message
> +               sprintf(cmd, "AT+CMGR=%s", cmti_pos);
> +               at2_send_modem_command(privdata, cmd, 7, 0);
> +               // and delete it
> +               sprintf(cmd, "AT+CMGD=%s", cmti_pos);
> +               at2_send_modem_command(privdata, cmd, 7, 0);
> +               continue;
> +           }
>              if (octstr_search(line, octstr_imm("+CMT:"), 0) != -1 ||
>                 octstr_search(line, octstr_imm("+CDS:"), 0) != -1 ||
>                  ((octstr_search(line, octstr_imm("+CMGR:"), 
> 0) != -1) && (cmgr_flag = 1)) ) {
>                    
> 
> -- 
> Best regards,
>  Rene                          mailto:[email protected]
> 
> 
>