Re: [RFC PATCH 5/5] smsutil: save pending status reports to imsi file

Hi Denis, I'm the guy, who already introduced shortly yesterday to Inaky, and I'm continuing Pasi Miettinen's fine work with SMS status-reports.

So, here are some comments (included below) for updating status report-functionality for your earlier comments, how SR-functionality could be improved.

> General comment here is that you're duplicating much of the code that > inserts > the information into the data structure. You can play the same trick as > sms_assembly which uses a single function to do the heavy lifting, > but takes > an argument whether to store this info on disk. Obviously during > load you > don't want to re-save this information on disk. Then your load function > becomes much smaller and easier to understand. > OK. I agree that generally single function for heavy lifting is nicer solution. I will implement some general function, which handles adding of new id-table (if not already added) to assembly-table, and storing necessary node-information there. After that I will move some functionality from sr_assembly_load_backup() to this general function. Also this function can be used when sending new sms-text message. And I will add new parameter for telling, if this function will save data to imsi-file on disk or not. Existing sms-assembly handles it that way. But, one difference with sms-assembly and status-report logic is, that sms-assembly naturally stores every messages of concatenated, long SMS-message to own backup files, but status report gathers all information belonging to the same long SMS-message (using message reference numbers) to the single file. So Pasi implemented in the loading-phase (sr_assembly_load_backup) storing of MR-numbers simply by copying the whole buffer containing MR's belonging to the single concatenated message-node: 1) memcpy(node->mrs, buf, sizeof(node->mrs)); And naturally when sending single message, single MR-bit is set up ('status_report_assembly_add_fragment()'): 2) node->mrs[offset] |= bit; So, when now implementing new general function, one solution could be to bring MR-string as parameter so way (1) could be used in different cases. Also I have to take care of variable 'sent_mrs', which will be incremented in the sms-submit, but not in the loading phase.

>> return ret; >> } >> >> +static gboolean sr_assembly_add_fragment_backup(const char *imsi, >> + const struct id_table_node *node, >> + unsigned int msg_id) > > Lets be consistent with how sms assembly works. E.g. take the main > assembly > object and return void. Just helps to read the code when it is > consistent. > OK. Lets give main assembly to this (and corresponding) function(s). How about return value? For instance existing sms assembly-function 'sms_assembly_add_fragment_backup()' already returns something else than void.