Re: [RFC PATCH 5/6] Support for concatenated SMS status report.

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Pasi,

> ---
>  include/history.h |    2 +
>  src/sms.c         |   67 ++++++++++++++++++++----
>  src/smsutil.c     |  152
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/smsutil.h     
| 
>   32 +++++++++++

Please break this up into at least 3 patches:
- include/history api change
- smsutil changes
- sms core atom changes

Ideally I'd also like to see a unit test for smsutil parts in the series.
> +void add_pending_status_report(GSList **pending_status_reports, const int
>  mr, +					const unsigned int msg_id,
> +					const struct sms_address *receiver);
> +
> +void free_pending_status_report(struct pending_status_report
>  *status_report, +				GSList **pending_status_reports);
> +
> +enum ofono_history_sms_status update_pending_status_report_mr_number(
> +		GSList **pending_status_reports, const guint8 mr,
> +		const struct sms_address *receiver,
> +		const enum sms_status_report_result status_report_result,
> +		unsigned int *relating_msg_id);
> +

So my first suggestion here is to make the status report assembly into its own 
class.  This way you can mess around with the internal data structures and 
optimize them later if needed without breaking the rest of the code.

This is also where unit tests come in. Once you have something working, you 
can optimize and make sure things still work... without affecting the rest of 
the code.

I suggest thinking about the data structure use some more.  In particular, 
using a list of struct mr_numbers is very inefficient.  Using a pair of bitmaps 
for mr numbers (which can only be 0..255) would be way more efficient and 
probably would make the code much easier to write.

Consider if making multi-level data structures would increase efficiency.  E.g. 
Hastable[destination_address] = list of structures mapping between message id 
and message references.  Or anything else efficient you can come up with.

Having said that, don't over-complicate it in the beginning.  Having something 
good enough and working is better.

For the object interface I suggest something like:

struct status_report_assembly;

- Creates a new assembly object, and populates pending status reports from the 
imsi-keyed store.  If imsi is NULL, no status reports are saved or loaded from 
disk.  This is useful for pseudo-modems:
struct status_report_assembly *status_report_assembly_new(const char *imsi);

- Frees the assembly:
void status_report_assembly_free(struct status_report_assembly *assembly);

- Just a thought, but some networks simply don't support status reports, so we 
need to expire status reports for messages that have lived past their validity 
period:
void status_report_assembly_expire(struct status_report_assembly *assembly,
								time_t before, (GFunc) foreach_func,
								gpointer data);

- Add a status report to the assembly and return whether this resulted in a 
message being delivered / not:
gboolean status_report_assembly_report(struct status_report_assembly 
*assembly,
										const struct sms *status_report,
										unsigned int *msg_id,
										enum sms_st *status)

- Add a message fragment to the assembly for tracking:
void status_report_assembly_add_fragment(struct status_report_assembly 
*assembly,
										unsigned int *msg_id,
										struct sms_address *to,
										unsigned char mr,
										time_t expiration);

Feel free to modify the above as needed, this is just something I came up with 
while reviewing your patch.

Regards,
-Denis