Re: [SMS D-Bus 03/19] sms: implement SHA256-based message IDs [incomplete]

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8021 bytes --]

Hi Inaky,

On 08/03/2010 06:50 PM, Inaky Perez-Gonzalez wrote:
> From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> 
> This patch removes the sequential SMS message identification gig and
> replaces it with a 16-bit hash cookie based off message contents.
> 
> FIXME: [incomplete] need to figure out how to do so that identical
> messages sent to different or the same numbers also have a different
> ID.
> ---
>  src/ofono.h |    9 +++++----
>  src/sms.c   |   55 +++++++++++++++++++++++--------------------------------
>  src/stk.c   |    5 +++--
>  3 files changed, 31 insertions(+), 38 deletions(-)
> 
> diff --git a/src/ofono.h b/src/ofono.h
> index aaa01d9..b73797b 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -185,10 +185,11 @@ enum ofono_sms_submit_flag {
>  
>  typedef void (*ofono_sms_txq_submit_cb_t)(gboolean ok, void *data);
>  
> -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
> -					unsigned int flags,
> -					ofono_sms_txq_submit_cb_t cb,
> -					void *data, ofono_destroy_func destroy);
> +struct tx_queue_entry *__ofono_sms_txq_submit(
> +	struct ofono_sms *sms, GSList *list,
> +	unsigned int flags, unsigned ref,
> +	ofono_sms_txq_submit_cb_t cb,
> +	void *data, ofono_destroy_func destroy);

As I said before, please don't mess with this function.

>  
>  #include <ofono/sim.h>
>  #include <ofono/stk.h>
> diff --git a/src/sms.c b/src/sms.c
> index 35364db..39d4a32 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -55,7 +55,6 @@ struct ofono_sms {
>  	DBusMessage *pending;
>  	struct ofono_phone_number sca;
>  	struct sms_assembly *assembly;
> -	unsigned int next_msg_id;
>  	guint ref;
>  	GQueue *txq;
>  	gint tx_source;
> @@ -589,7 +588,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
>  	int ref_offset;
>  	struct ofono_modem *modem;
>  	unsigned int flags;
> -	unsigned int msg_id;
> +	guint16 msg_id;
> +	struct tx_queue_entry *sms_msg;
>  
>  	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
>  					DBUS_TYPE_STRING, &text,
> @@ -605,24 +605,19 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
>  	if (!msg_list)
>  		return __ofono_error_invalid_format(msg);
>  
> -	set_ref_and_to(msg_list, sms->ref, ref_offset, to);
> -	DBG("ref: %d, offset: %d", sms->ref, ref_offset);
> -
> -	if (ref_offset != 0) {
> -		if (sms->ref == 65536)
> -			sms->ref = 1;
> -		else
> -			sms->ref = sms->ref + 1;
> -	}
> +	msg_id = sms_uuid_from_pdu_list(msg_list);

You're calculating the ID before setting the ref and to? That is pretty
silly ;)

> +	set_ref_and_to(msg_list, msg_id, ref_offset, to);
> +	DBG("SMS ID: 0x%04x", msg_id);
>  
>  	flags = OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY;
>  	flags |= OFONO_SMS_SUBMIT_FLAG_RETRY;
>  	if (sms->use_delivery_reports)
>  		flags |= OFONO_SMS_SUBMIT_FLAG_REQUEST_SR;
>  
> -	msg_id = __ofono_sms_txq_submit(sms, msg_list, flags, send_message_cb,
> -						dbus_message_ref(msg),
> -						send_message_destroy);
> +	sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id,
> +					 send_message_cb,
> +					 dbus_message_ref(msg),
> +					 send_message_destroy);

Make the ID calculation happen inside tx_queue_entry_new.  That function
already converts all the SMS objects into a PDU.  So simply hashing the
PDUs there would be very easy.

>  
>  	g_slist_foreach(msg_list, (GFunc)g_free, NULL);
>  	g_slist_free(msg_list);
> @@ -659,7 +654,7 @@ static void dispatch_app_datagram(struct ofono_sms *sms, int dst, int src,
>  }
>  
>  static void dispatch_text_message(struct ofono_sms *sms,
> -					const char *message,
> +					const char *message, guint16 msg_id,
>  					enum sms_class cls,
>  					const struct sms_address *addr,
>  					const struct sms_scts *scts)
> @@ -717,11 +712,9 @@ static void dispatch_text_message(struct ofono_sms *sms,
>  
>  	g_dbus_send_message(conn, signal);
>  
> -	if (cls != SMS_CLASS_0) {
> -		__ofono_history_sms_received(modem, sms->next_msg_id, str,
> +	if (cls != SMS_CLASS_0)
> +		__ofono_history_sms_received(modem, msg_id, str,
>  						&remote, &local, message);
> -		sms->next_msg_id += 1;
> -	}
>  }
>  
>  static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
> @@ -732,6 +725,7 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
>  	enum sms_class cls;
>  	int srcport = -1;
>  	int dstport = -1;
> +	guint16 msg_id;
>  
>  	if (sms_list == NULL)
>  		return;
> @@ -749,6 +743,7 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
>  	 * used, the addresses are the same across all segments.
>  	 */
>  
> +	msg_id = sms_uuid_from_pdu_list(sms_list);
>  	for (l = sms_list; l; l = l->next) {
>  		guint8 dcs;
>  		gboolean comp = FALSE;
> @@ -825,8 +820,8 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
>  
>  		s = sms_list->data;
>  
> -		dispatch_text_message(sms, message, cls, &s->deliver.oaddr,
> -					&s->deliver.scts);
> +		dispatch_text_message(sms, message, msg_id, cls,
> +					&s->deliver.oaddr, &s->deliver.scts);
>  		g_free(message);
>  	}
>  }
> @@ -1104,8 +1099,6 @@ static void sms_remove(struct ofono_atom *atom)
>  
>  	if (sms->settings) {
>  		g_key_file_set_integer(sms->settings, SETTINGS_GROUP,
> -					"NextMessageId", sms->next_msg_id);
> -		g_key_file_set_integer(sms->settings, SETTINGS_GROUP,
>  					"NextReference", sms->ref);
>  		g_key_file_set_boolean(sms->settings, SETTINGS_GROUP,
>  					"UseDeliveryReports",
> @@ -1201,9 +1194,6 @@ static void sms_load_settings(struct ofono_sms *sms, const char *imsi)
>  
>  	sms->imsi = g_strdup(imsi);
>  
> -	sms->next_msg_id = g_key_file_get_integer(sms->settings, SETTINGS_GROUP,
> -							"NextMessageId", NULL);
> -
>  	sms->ref = g_key_file_get_integer(sms->settings, SETTINGS_GROUP,
>  							"NextReference", NULL);
>  	if (sms->ref >= 65536)
> @@ -1306,10 +1296,11 @@ void *ofono_sms_get_data(struct ofono_sms *sms)
>  	return sms->driver_data;
>  }
>  
> -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
> -					unsigned int flags,
> -					ofono_sms_txq_submit_cb_t cb,
> -					void *data, ofono_destroy_func destroy)
> +struct tx_queue_entry *__ofono_sms_txq_submit(
> +	struct ofono_sms *sms, GSList *list,
> +	unsigned int flags, unsigned msg_id,
> +	ofono_sms_txq_submit_cb_t cb,
> +	void *data, ofono_destroy_func destroy)
>  {
>  	struct tx_queue_entry *entry = tx_queue_entry_new(list);
>  
> @@ -1320,7 +1311,7 @@ unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>  				sizeof(entry->receiver));
>  	}
>  
> -	entry->msg_id = sms->next_msg_id++;
> +	entry->msg_id = msg_id;
>  	entry->flags = flags;
>  	entry->cb = cb;
>  	entry->data = data;
> @@ -1331,5 +1322,5 @@ unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>  	if (g_queue_get_length(sms->txq) == 1)
>  		sms->tx_source = g_timeout_add(0, tx_next, sms);
>  
> -	return entry->msg_id;
> +	return entry;
>  }
> diff --git a/src/stk.c b/src/stk.c
> index 556dc68..620811b 100644
> --- a/src/stk.c
> +++ b/src/stk.c
> @@ -323,6 +323,7 @@ static gboolean handle_command_send_sms(const struct stk_command *cmd,
>  	struct ofono_atom *sms_atom;
>  	struct ofono_sms *sms;
>  	GSList msg_list;
> +	guint16 msg_id;
>  
>  	sms_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SMS);
>  
> @@ -338,8 +339,8 @@ static gboolean handle_command_send_sms(const struct stk_command *cmd,
>  
>  	msg_list.data = (void *) &cmd->send_sms.gsm_sms;
>  	msg_list.next = NULL;
> -
> -	__ofono_sms_txq_submit(sms, &msg_list, 0, send_sms_submit_cb,
> +	msg_id = sms_uuid_from_pdu_list(&msg_list);
> +	__ofono_sms_txq_submit(sms, &msg_list, 0, msg_id, send_sms_submit_cb,
>  				stk->sms_submit_req, g_free);
>  
>  	stk->cancel_cmd = send_sms_cancel;

Regards,
-Denis