Hi Andrew,
On Fri, Aug 21, 2009 at 9:40 PM, Andrzej Zaborowski <
andrew.zaborow...@intel.com> wrote:
> This way we can continue receiving segmented messages over a reset or
> crash.
> ---
> src/common.c | 29 +++++++
> src/common.h | 10 +++
> src/sim.c | 34 --------
> src/sms.c | 14 +++-
> src/smsutil.c | 237
> +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> src/smsutil.h | 3 +-
> unit/Makefile.am | 6 +-
> unit/test-sms.c | 4 +-
> 8 files changed, 293 insertions(+), 44 deletions(-)
>
>
Somehow this patch fell through the cracks completely. Can you rebase and
resend?
> +
> +int create_dirs(const char *filename, const mode_t mode)
> +{
>
Can we move this part to storage.c instead of common.c to be more consistent
with connman & bluez?
> +
> +#ifdef TEMP_FAILURE_RETRY
> +#define TFR TEMP_FAILURE_RETRY
> +#else
> +#define TFR
> +#endif
> +
> +#include <fcntl.h>
> +
> +int create_dirs(const char *filename, const mode_t mode);
>
storage.c as well.
> +static GSList *sms_assembly_add_fragment_backup(struct sms_assembly
> *assembly,
> + const struct sms *sms, time_t ts,
> + const struct sms_address *addr,
> + guint16 ref, guint8 max, guint8
> seq,
> + gboolean backup);
> +
> +#define SMS_BACKUP_MODE 0600
> +#define SMS_BACKUP_PATH STORAGEDIR "/%s/sms"
> +#define SMS_BACKUP_PATH_DIR SMS_BACKUP_PATH "/%s-%i-%i"
> +#define SMS_BACKUP_PATH_FILE SMS_BACKUP_PATH_DIR "/%03i"
> +
> +#define SMS_ADDR_FMT "%21[0-9+*#]"
>
These really belong at the top of the file. Includes, Defines, globals,
forward declarations, structure definitions in that order.
> +
> +static void sms_assembly_load(struct sms_assembly *assembly,
> + const struct dirent *dir)
> +{
> + for (; len--; free(segments[len])) {
>
Please don't do such evilness. I don't even understand how this can work.
> + fd = TFR(open(path, O_RDONLY));
> + g_free(path);
> +
> + if (fd == -1)
> + continue;
> +
> + if (fstat(fd, &segment_stat) != 0) {
> + TFR(close(fd));
> + continue;
> + }
> +
> + r = TFR(read(fd, buf, sizeof(buf)));
>
This part should be a utility function in storage.c. We repeat almost the
same code in sim.c
> +
> + if (r > 0 && sms_deserialize(buf, &segment, r)) {
> + if (sms_assembly_add_fragment_backup(assembly,
> + &segment,
> + segment_stat.st_mtime,
> + &addr, ref, max, seq,
> FALSE)) {
> + /* This can't happen */
> + }
>
We read it from the backup store and almost immediately overwrite the backup
store? Seems inefficient.
> + if (create_dirs(path, SMS_BACKUP_MODE | S_IXUSR)) {
> + g_free(path);
> + return FALSE;
> + }
> +
> + fd = TFR(open(path, O_WRONLY | O_CREAT, SMS_BACKUP_MODE));
> + if (fd == -1) {
> + g_free(path);
> + return FALSE;
> + }
> +
> + if (TFR(write(fd, buf, len)) < len) {
> + TFR(close(fd));
> + unlink(path);
> + g_free(path);
> + return FALSE;
> + }
> +
> + g_free(path);
> + TFR(close(fd));
>
Again, sounds like this should be a utility function in storage.c
Regards,
-Denis
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
