Re: [PATCH]: HTTP reply from sendota fixes (#1)

Hi,

patch committed to cvs.

Thanks,
Alex

Dziugas Baltrunas schrieb:
> Hi,
> 
> I kept this in mind, but I was trying to follow current indenting of
> the relevant blocks which is already messed quite a lot, otherwise
> patch would became irrelevant for the fix, but indents.
> 
> Anyway, fixed some of them.
> 
> On 3/23/06, Alexander Malysh <[email protected]> wrote:
>> Hi,
>>
>> patch looks good. one thing: please fix coding style (indents).
>> Otherwise I'm +1 for it.
>>
>> Thanks,
>> Alex
>>
>> Dziugas Baltrunas wrote:
>>
>>> Hi again,
>>>
>>> any objections on this? I've already tested the patch on bigger loads
>>> and it seems to work fine.
>>>
>>> Sometimes you can see "Delayed reply - wait for bearerbox" after " Got
>>> ACK" in the logs, but this is obvious since we print the message
>>> before puting client into dict.
>>>
>>> On 3/21/06, Dziugas Baltrunas <[email protected]> wrote:
>>>> Hi,
>>>>
>>>> attached is an attempt to follow Alexander's advice long time ago for
>>>> Kalle to put client into dict before sending a message to bearerbox.
>>>> Not sure if it will work in all cases, so additional testing is
>>>> required. Patch also includes fix for freeing msg which I sent before.
>>>>
>>>> To be more precise, I'm not sure if dict_remove(client_dict,
>>>> stored_uuid) in my patch is necessary since smsbox.c:send_message()
>>>> _never_ returns -1, although it's stated so and there are such checks
>>>> in the code. This becomes more obvious when we look to
>>>> write_to_bearerbox(), which returns void, so we actually never really
>>>> know, whether bearerbox accepted the message or not.
>>>>
>>>> Thanks.
>>>>
>>>> On 3/21/06, Dziugas Baltrunas <[email protected]> wrote:
>>>>> Hi, list,
>>>>>
>>>>> attached patch fixes freeing of msg structure in smsbox.c sendota
>>>>> stuff before extracting it's uuid with store_uuid(). Without patch
>>>>> applied you can see the following in the logs:
>>>>>
>>>>> 2006-03-20 18:35:36 [29721] [3] DEBUG: Stored UUID
>>>>> 00000000-0000-0000-0000-000000000000
>>>>>
>>>>> However, current patch does not solve HTTP reply from sendota
>>>>> completely, since it's related how fast we get an ACK from bearerbox.
>>>>> If we're successful, we'll get "0: Accepted for delivery", and if not,
>>>>> no answer will be sent at all. This can be illustrated by these
>>>>> snippets:
>>>>>
>>>>> 1. We got an ACK from bearerbox (read_messages_from_bearerbox())
>>>>> before store_uuid() was called:
>>>>>
>>>>> 2006-03-21 12:47:37 [19015] [3] INFO: /cgi-bin/sendota <<default>>
>>>>> <XXX> 2006-03-21 12:47:37 [19015] [3] DEBUG: message length 446,
>>>>> sending 4 messages 2006-03-21 12:47:37 [19015] [0] DEBUG: Got ACK (0)
>>>>> of 8b37ca04-896f-4510-8d2d-000289b6b8b2
>>>>> 2006-03-21 12:47:37 [19015] [0] DEBUG: No client - multi-send or ACK
>>>>> to pull-reply
>>>>> 2006-03-21 12:47:37 [19015] [3] DEBUG: Stored UUID
>>>>> 8b37ca04-896f-4510-8d2d-000289b6b8b2
>>>>> 2006-03-21 12:47:37 [19015] [3] DEBUG: Status: 202 Answer: <Sent.>
>>>>> 2006-03-21 12:47:37 [19015] [3] DEBUG: Delayed reply - wait for
>>>>> bearerbox
>>>>>
>>>>> 2. We got an ACK from bearerbox after store_uuid() was called (as
>>>>> expected):
>>>>>
>>>>> 2006-03-21 12:47:09 [19015] [3] INFO: /cgi-bin/sendota <<default>>
>>>>> <XXX> 2006-03-21 12:47:09 [19015] [3] DEBUG: message length 446,
>>>>> sending 4 messages 2006-03-21 12:47:09 [19015] [3] DEBUG: Stored UUID
>>>>> f6b0cb63-b561-49fa-951b-09aa5177067e
>>>>> 2006-03-21 12:47:09 [19015] [3] DEBUG: Status: 202 Answer: <Sent.>
>>>>> 2006-03-21 12:47:09 [19015] [3] DEBUG: Delayed reply - wait for
>>>>> bearerbox 2006-03-21 12:47:09 [19015] [0] DEBUG: Got ACK (0) of
>>>>> f6b0cb63-b561-49fa-951b-09aa5177067e
>>>>>
>>>>> This is relevant only when immediate_sendsms_reply is false (which is
>>>>> default!), so I should ask Kalle,  the author of introducing this
>>>>> feature, comment on possible thread synchronisation issues, which IMHO
>>>>> sometimes could also arise for ordinary sendsms request.
>>>>>
>>>>> Thanks,
>>>>> Dziugas
>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> Dziugas
>>>>
>>>>
>>>>
>>>
>>> --
>>> Dziugas
>> --
>> Thanks,
>> Alex
>>
>>
>>
> 
> 
> --
> Dziugas
> 
> 
> ------------------------------------------------------------------------
> 
> Index: smsbox.c
> ===================================================================
> RCS file: /home/cvs/gateway/gw/smsbox.c,v
> retrieving revision 1.260
> diff -u -r1.260 smsbox.c
> --- smsbox.c	9 Dec 2005 02:14:31 -0000	1.260
> +++ smsbox.c	21 Mar 2006 13:09:33 -0000
> @@ -1958,7 +1958,7 @@
>  
>  
>  static Octstr *smsbox_req_handle(URLTranslation *t, Octstr *client_ip,
> -				 Octstr **stored_uuid,
> +				 HTTPClient *client,
>  				 Octstr *from, Octstr *to, Octstr *text, 
>  				 Octstr *charset, Octstr *udh, Octstr *smsc,
>  				 int mclass, int mwi, int coding, int compress, 
> @@ -1969,6 +1969,7 @@
>  {				     
>      Msg *msg = NULL;
>      Octstr *newfrom, *returnerror, *receiv;
> +    Octstr *stored_uuid = NULL;
>      List *failed_id, *allowed, *denied;
>      int no_recv, ret = 0, i;
>      long del;
> @@ -2292,6 +2293,11 @@
>       */
>      failed_id = gwlist_create();
>  
> +    if (!immediate_sendsms_reply) {
> +        stored_uuid = store_uuid(msg);
> +        dict_put(client_dict, stored_uuid, client);
> +    }
> +
>      while ((receiv = gwlist_extract_first(allowed)) != NULL) {
>  
>  	O_DESTROY(msg->sms.receiver);
> @@ -2313,13 +2319,6 @@
>  	             udh == NULL ? ( text == NULL ? "" : octstr_get_cstr(text) ) : "<< UDH >>");
>          }
>      }
> -    /* Store id if needed for a delayed HTTP reply */
> -
> -    if (!immediate_sendsms_reply) {
> -	*stored_uuid = store_uuid(msg);
> -    }
> -    
> -
>  
>      msg_destroy(msg);
>      gwlist_destroy(receiver, octstr_destroy_item);
> @@ -2377,6 +2376,8 @@
>      octstr_destroy(from);
>      *status = HTTP_INTERNAL_SERVER_ERROR;
>      returnerror = octstr_create("Sending failed.");
> +    if (!immediate_sendsms_reply)
> +        dict_remove(client_dict, stored_uuid);
>  
>      /* 
>       * Append all receivers to the returned body in case this is
> @@ -2389,6 +2390,8 @@
>          }
>      }
>  
> +    if (stored_uuid)
> +        octstr_destroy(stored_uuid);
>      octstr_destroy(receiv); 
>      gwlist_destroy(failed_id, octstr_destroy_item);
>      gwlist_destroy(denied, octstr_destroy_item);
> @@ -2470,7 +2473,7 @@
>   * Args: args contains the CGI parameters
>   */
>  static Octstr *smsbox_req_sendsms(List *args, Octstr *client_ip, int *status,
> -				  Octstr **stored_uuid)
> +				  HTTPClient *client)
>  {
>      URLTranslation *t = NULL;
>      Octstr *tmp_string;
> @@ -2568,7 +2571,7 @@
>  	return octstr_create("Empty receiver number not allowed, rejected");
>      }
>  
> -    return smsbox_req_handle(t, client_ip, stored_uuid, from, to, text, charset, udh,
> +    return smsbox_req_handle(t, client_ip, client, from, to, text, charset, udh,
>  			     smsc, mclass, mwi, coding, compress, validity, 
>  			     deferred, status, dlr_mask, dlr_url, account,
>  			     pid, alt_dcs, rpi, NULL, binfo, priority);
> @@ -2582,7 +2585,7 @@
>   */
>  static Octstr *smsbox_sendsms_post(List *headers, Octstr *body,
>  				   Octstr *client_ip, int *status,
> -				   Octstr **stored_uuid)
> +				   HTTPClient *client)
>  {
>      URLTranslation *t = NULL;
>      Octstr *user, *pass, *ret, *type;
> @@ -2678,7 +2681,7 @@
>  	}
>  
>  	if (ret == NULL)
> -	    ret = smsbox_req_handle(t, client_ip, stored_uuid, from, to, body, charset,
> +	    ret = smsbox_req_handle(t, client_ip, client, from, to, body, charset,
>  				    udh, smsc, mclass, mwi, coding, compress, 
>  				    validity, deferred, status, dlr_mask, 
>  				    dlr_url, account, pid, alt_dcs, rpi, tolist,
> @@ -2789,10 +2792,12 @@
>   * Args: list contains the CGI parameters
>   */
>  static Octstr *smsbox_req_sendota(List *list, Octstr *client_ip, int *status,
> -				  Octstr **stored_uuid)
> +				  HTTPClient *client)
>  {
>      Octstr *id, *from, *phonenumber, *smsc, *ota_doc, *doc_type, *account;
>      CfgGroup *grp;
> +    Octstr *returnerror;
> +    Octstr *stored_uuid = NULL;
>      List *grplist;
>      Octstr *p;
>      URLTranslation *t;
> @@ -2944,21 +2949,29 @@
>      info(0, "%s <%s> <%s>", octstr_get_cstr(sendota_url), 
>      	 id ? octstr_get_cstr(id) : "<default>", octstr_get_cstr(phonenumber));
>  
> +    if (!immediate_sendsms_reply) {
> +        stored_uuid = store_uuid(msg);
> +        dict_put(client_dict, stored_uuid, client);
> +    }
> +
>      ret = send_message(t, msg); 
> -    msg_destroy(msg);
>  
>      if (ret == -1) {
>          error(0, "sendota_request: failed");
>          *status = HTTP_INTERNAL_SERVER_ERROR;
> -        return octstr_create("Sending failed.");
> -    }
> -    else if (!immediate_sendsms_reply) {
> -	*stored_uuid = store_uuid(msg);
> +        returnerror = octstr_create("Sending failed.");
> +        dict_remove(client_dict, stored_uuid);
> +    } else {
> +        *status = HTTP_ACCEPTED;
> +        returnerror = octstr_create("Sent.");
>      }
>  
> -    
> -    *status = HTTP_ACCEPTED;
> -    return octstr_create("Sent.");
> +    msg_destroy(msg);
> +
> +    if (stored_uuid)
> +        octstr_destroy(stored_uuid);
> +
> +    return returnerror;
>  }
>  
>  
> @@ -2972,11 +2985,12 @@
>   */
>  static Octstr *smsbox_sendota_post(List *headers, Octstr *body,
>                                     Octstr *client_ip, int *status,
> -				   Octstr **stored_uuid)
> +				   HTTPClient *client)
>  {
>      Octstr *name, *val, *ret;
>      Octstr *from, *to, *id, *user, *pass, *smsc;
>      Octstr *type, *charset, *doc_type, *ota_doc, *sec, *pin;
> +    Octstr *stored_uuid = NULL;
>      URLTranslation *t;
>      Msg *msg;
>      long l;
> @@ -3125,20 +3139,30 @@
>  	    info(0, "%s <%s> <%s>", octstr_get_cstr(sendota_url), 
>  		 id ? octstr_get_cstr(id) : "XML", octstr_get_cstr(to));
>      
> +
> +        if (!immediate_sendsms_reply) {
> +            stored_uuid = store_uuid(msg);
> +            dict_put(client_dict, stored_uuid, client);
> +        }
> +
>  	    r = send_message(t, msg); 
> -	    msg_destroy(msg);
>  
>  	    if (r == -1) {
> -		error(0, "sendota_request: failed");
> -		*status = HTTP_INTERNAL_SERVER_ERROR;
> -		ret = octstr_create("Sending failed.");
> -	    }
> -	    else if (!immediate_sendsms_reply) {
> -		*stored_uuid = store_uuid(msg);
> +            error(0, "sendota_request: failed");
> +            *status = HTTP_INTERNAL_SERVER_ERROR;
> +            ret = octstr_create("Sending failed.");
> +            if (!immediate_sendsms_reply) 
> +                dict_remove(client_dict, stored_uuid);
> +       } else  {
> +            *status = HTTP_ACCEPTED;
> +            ret = octstr_create("Sent.");
>  	    }
>  
> -	    *status = HTTP_ACCEPTED;
> -	    ret = octstr_create("Sent.");
> +       msg_destroy(msg);
> +
> +        if (stored_uuid)
> +            octstr_destroy(stored_uuid);
> +
>  	}
>      }    
>      
> @@ -3157,9 +3181,6 @@
>      Octstr *ip, *url, *body, *answer;
>      List *hdrs, *args;
>      int status;
> -    Octstr *stored_uuid;
> -
> -    stored_uuid = NULL;
>      
>      for (;;) {
>      	client = http_accept_request(sendsms_port, &ip, &url, &hdrs, &body, 
> @@ -3183,9 +3204,9 @@
>  	 * related routine handle the checking
>  	 */
>  	if (body == NULL)
> -	    answer = smsbox_req_sendsms(args, ip, &status, &stored_uuid);
> +	    answer = smsbox_req_sendsms(args, ip, &status, client);
>  	else
> -	    answer = smsbox_sendsms_post(hdrs, body, ip, &status, &stored_uuid);
> +	    answer = smsbox_sendsms_post(hdrs, body, ip, &status, client);
>      }
>      /* XML-RPC */
>      else if (octstr_compare(url, xmlrpc_url) == 0)
> @@ -3203,9 +3224,9 @@
>      else if (octstr_compare(url, sendota_url) == 0)
>      {
>  	if (body == NULL)
> -            answer = smsbox_req_sendota(args, ip, &status, &stored_uuid);
> +            answer = smsbox_req_sendota(args, ip, &status, client);
>          else
> -            answer = smsbox_sendota_post(hdrs, body, ip, &status, &stored_uuid);
> +            answer = smsbox_sendota_post(hdrs, body, ip, &status, client);
>      }
>      /* add aditional URI compares here */
>      else {
> @@ -3222,12 +3243,10 @@
>  	octstr_destroy(body);
>  	http_destroy_cgiargs(args);
>  
> -	if (immediate_sendsms_reply || status != HTTP_ACCEPTED || stored_uuid == NULL)
> +	if (immediate_sendsms_reply || status != HTTP_ACCEPTED)
>  	  http_send_reply(client, status, sendsms_reply_hdrs, answer);
>  	else {
>  	  debug("sms.http", 0, "Delayed reply - wait for bearerbox");
> -	  dict_put(client_dict, stored_uuid, client);
> -	  octstr_destroy(stored_uuid);
>  	}
>  	octstr_destroy(answer);
>      }
> 
>