Re: [PATCH 1/3] sms: store pending tx pdus on disk

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Kristen,

On 11/24/2010 04:53 PM, Kristen Carlson Accardi wrote:
> ---
>  src/sms.c     |   17 +++++++++++++++++
>  src/smsutil.c |   26 ++++++++++++++++++++++++++
>  src/smsutil.h |    3 +++
>  3 files changed, 46 insertions(+), 0 deletions(-)
> 
> diff --git a/src/sms.c b/src/sms.c
> index 12988c8..6eab0fa 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -52,6 +52,8 @@ static gboolean tx_next(gpointer user_data);
>  
>  static GSList *g_drivers = NULL;
>  
> +static unsigned long tx_counter = 0;
> +

I think that using a static variable for this is a bad idea.  You
probably want to add this to the sim atom itself.  More on this a bit later.

>  enum message_state {
>  	MESSAGE_STATE_PENDING =		0,
>  	MESSAGE_STATE_SENT,
> @@ -111,6 +113,7 @@ struct tx_queue_entry {
>  	ofono_sms_txq_submit_cb_t cb;
>  	void *data;
>  	ofono_destroy_func destroy;
> +	int id;

Is there a reason for the type of id and the type of tx_counter to be
different?

>  };
>  
>  static gboolean uuid_equal(gconstpointer v1, gconstpointer v2)
> @@ -1899,6 +1902,8 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>  {
>  	struct message *m = NULL;
>  	struct tx_queue_entry *entry;
> +	int i;
> +	GSList *l;
>  
>  	entry = tx_queue_entry_new(list, flags);
>  	if (entry == NULL)
> @@ -1923,6 +1928,8 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>  			sms->ref = sms->ref + 1;
>  	}
>  
> +	entry->id = tx_counter++;
> +
>  	g_queue_push_tail(sms->txq, entry);
>  
>  	if (g_queue_get_length(sms->txq) == 1)
> @@ -1931,6 +1938,16 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>  	if (uuid)
>  		memcpy(uuid, &entry->uuid, sizeof(*uuid));
>  
> +	i = 0;
> +
> +	for (l = list; l; l = l->next) {

I'd really prefer i = 0, l = list here

> +		struct sms *s = l->data;
> +		sms_tx_store(sms->imsi, entry->id,
> +				ofono_uuid_to_str(&entry->uuid),
> +				s, i);
> +		i++;
> +	}
> +
>  	if (cb)
>  		cb(sms, &entry->uuid, data);
>  
> diff --git a/src/smsutil.c b/src/smsutil.c
> index e6dbf5f..4904419 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -48,6 +48,10 @@
>  #define SMS_SR_BACKUP_PATH STORAGEDIR "/%s/sms_sr"
>  #define SMS_SR_BACKUP_PATH_FILE SMS_SR_BACKUP_PATH "/%s-%s"
>  
> +#define SMS_TX_BACKUP_PATH STORAGEDIR "/%s/tx_queue"
> +#define SMS_TX_BACKUP_PATH_DIR SMS_TX_BACKUP_PATH "/%lu-%s"
> +#define SMS_TX_BACKUP_PATH_FILE SMS_TX_BACKUP_PATH_DIR "/%03i"
> +
>  #define SMS_ADDR_FMT "%24[0-9A-F]"
>  #define SMS_MSGID_FMT "%40[0-9A-F]"
>  
> @@ -3125,6 +3129,28 @@ void status_report_assembly_expire(struct status_report_assembly *assembly,
>  	}
>  }
>  
> +gboolean sms_tx_store(const char *imsi, unsigned long id, const char *uuid,
> +			struct sms *s, guint8 seq)
> +{
> +	int len;
> +	unsigned char buf[177];
> +
> +	if (!imsi)
> +		return FALSE;
> +
> +	len = sms_serialize(buf, s);
> +
> +	/*
> +	 * file name is: imsi/order in tx_queue-uuid/order of pdus
> +	 */
> +	if (write_file(buf, len, SMS_BACKUP_MODE,
> +				SMS_TX_BACKUP_PATH_FILE, imsi, id, uuid,
> +				seq) != len)
> +		return FALSE;
> +

And where are you taking care of storing the other needed information?
e.g. the tx_queue entry flags, retry count, etc?

> +	return TRUE;
> +}
> +
>  static inline GSList *sms_list_append(GSList *l, const struct sms *in)
>  {
>  	struct sms *sms;
> diff --git a/src/smsutil.h b/src/smsutil.h
> index 4b05313..5279489 100644
> --- a/src/smsutil.h
> +++ b/src/smsutil.h
> @@ -516,6 +516,9 @@ void status_report_assembly_add_fragment(struct status_report_assembly
>  void status_report_assembly_expire(struct status_report_assembly *assembly,
>  					time_t before);
>  
> +gboolean sms_tx_store(const char *imsi, unsigned long id, const char *uuid,
> +				struct sms *s, guint8 seq);
> +
>  GSList *sms_text_prepare(const char *to, const char *utf8, guint16 ref,
>  				gboolean use_16bit,
>  				gboolean use_delivery_reports);

Otherwise I'm happy with this one.

Regards,
-Denis