Re: [patch] bearerbox acknowledged sendsms (fixed)

Hi Kalle,

some comments to your patch...

+/*
+ * Handle delayed reply to HTTP sendsms client, if any
+ */
+static void delayed_http_reply(Msg *msg)
+{
...
+    uuid_unparse(msg->ack.id, id);
+    os = octstr_create(id);
+    debug("sms.http", 0, "Got ACK (%d) of %s", msg->ack.nack, octstr_get_cstr(os));
+    client = dict_remove(client_dict, os);
+    if (client == NULL) {
+        debug("sms.http", 0, "No client - multi-send or ACK to pull-reply");
+        octstr_destroy(os);
+        return;
+    }
...

Destroy 'os' immediately after dict_remove instead of in if{} and later at
the end of function, it's not used later... then it's easier to undestand
that it not used anymore.

+static void store_uuid(Msg *msg, Octstr **stored_uuid)
+{
...

How about 'static Octstr *store_uuid(Msg *msg)'? I don't see any sense to
put Octstr** as function params...

...
+    dict_destroy(client_dict); /* XXX is this right location? */
+    http_destroy_headers(sendsms_reply_hdrs); /* ditto */
...

why not test if it's a right location todo it instead of writing such
comment? IMO such comment is useless.


Otherwise patch looks good although not tested yet.


Kalle Marjola wrote:

> (patch attached)
> 
> Why am I using this FC2 when all it causes is problems..
> 
> 


-- 
Thanks,
Alex