Hi Andrew,<br><br><div class="gmail_quote">On Fri, Aug 21, 2009 at 9:40 PM, Andrzej Zaborowski <span dir="ltr">&lt;<a href="mailto:andrew.zaborowski@intel.com" target="_blank">andrew.zaborowski@intel.com</a>&gt;</span> wrote:<br>

<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
This way we can continue receiving segmented messages over a reset or<br>
crash.<br>
---<br>
 src/common.c     |   29 +++++++<br>
 src/common.h     |   10 +++<br>
 src/sim.c        |   34 --------<br>
 src/sms.c        |   14 +++-<br>
 src/smsutil.c    |  237 +++++++++++++++++++++++++++++++++++++++++++++++++++++-<br>
 src/smsutil.h    |    3 +-<br>
 unit/Makefile.am |    6 +-<br>
 unit/test-sms.c  |    4 +-<br>
 8 files changed, 293 insertions(+), 44 deletions(-)<br>
<br></blockquote><div><br>Somehow this patch fell through the cracks completely.  Can you rebase and resend?<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">



+<br>
+int create_dirs(const char *filename, const mode_t mode)<br>
+{<br></blockquote><div><br>Can we move this part to storage.c instead of common.c to be more consistent with connman &amp; bluez?<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">



+<br>
+#ifdef TEMP_FAILURE_RETRY<br>
+#define TFR TEMP_FAILURE_RETRY<br>
+#else<br>
+#define TFR<br>
+#endif<br>
+<br>
+#include &lt;fcntl.h&gt;<br>
+<br>
+int create_dirs(const char *filename, const mode_t mode);<br></blockquote><div><br>storage.c as well.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">



+static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,<br>
+                                       const struct sms *sms, time_t ts,<br>
+                                       const struct sms_address *addr,<br>
+                                       guint16 ref, guint8 max, guint8 seq,<br>
+                                       gboolean backup);<br>
+<br>
+#define SMS_BACKUP_MODE 0600<br>
+#define SMS_BACKUP_PATH STORAGEDIR &quot;/%s/sms&quot;<br>
+#define SMS_BACKUP_PATH_DIR SMS_BACKUP_PATH &quot;/%s-%i-%i&quot;<br>
+#define SMS_BACKUP_PATH_FILE SMS_BACKUP_PATH_DIR &quot;/%03i&quot;<br>
+<br>
+#define SMS_ADDR_FMT &quot;%21[0-9+*#]&quot;<br></blockquote><div><br>These really belong at the top of the file.  Includes, Defines, globals, forward declarations, structure definitions in that order.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

+<br>
+static void sms_assembly_load(struct sms_assembly *assembly,<br>
+                               const struct dirent *dir)<br>
+{<br>
+       for (; len--; free(segments[len])) {<br></blockquote><div><br>Please don&#39;t do such evilness.  I don&#39;t even understand how this can work.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+               fd = TFR(open(path, O_RDONLY));<br>
+               g_free(path);<br>
+<br>
+               if (fd == -1)<br>
+                       continue;<br>
+<br>
+               if (fstat(fd, &amp;segment_stat) != 0) {<br>
+                       TFR(close(fd));<br>
+                       continue;<br>
+               }<br>
+<br>
+               r = TFR(read(fd, buf, sizeof(buf)));<br></blockquote><div><br>This part should be a utility function in storage.c.  We repeat almost the same code in sim.c<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

+<br>
+               if (r &gt; 0 &amp;&amp; sms_deserialize(buf, &amp;segment, r)) {<br>
+                       if (sms_assembly_add_fragment_backup(assembly,<br>
+                                               &amp;segment,<br>
+                                               segment_stat.st_mtime,<br>
+                                               &amp;addr, ref, max, seq, FALSE)) {<br>
+                               /* This can&#39;t happen */<br>
+                       }<br></blockquote><div><br>We read it from the backup store and almost immediately overwrite the backup store?  Seems inefficient.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

+       if (create_dirs(path, SMS_BACKUP_MODE | S_IXUSR)) {<br>
+               g_free(path);<br>
+               return FALSE;<br>
+       }<br>
+<br>
+       fd = TFR(open(path, O_WRONLY | O_CREAT, SMS_BACKUP_MODE));<br>
+       if (fd == -1) {<br>
+               g_free(path);<br>
+               return FALSE;<br>
+       }<br>
+<br>
+       if (TFR(write(fd, buf, len)) &lt; len) {<br>
+               TFR(close(fd));<br>
+               unlink(path);<br>
+               g_free(path);<br>
+               return FALSE;<br>
+       }<br>
+<br>
+       g_free(path);<br>
+       TFR(close(fd));<br></blockquote><div><br>Again, sounds like this should be a utility function in storage.c<br><br></div><div>Regards,<br>-Denis <br></div></div><br>