Hi Inaky, On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote: First of all, please refer to doc/coding-style.txt Section M5. Second, please actually check that your code passes make distcheck: CCLD unit/test-idmap CC unit/test-sms.o CC src/smsutil.o ../src/smsutil.c:40:17: error: bug.h: No such file or directory make[2]: *** [src/smsutil.o] Error 1 make[1]: *** [all] Error 2 make: *** [distcheck] Error 1 Why are you including this without actually using it? Regards, -Denis

Hi Inaky, On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote: Please do me a favor and add an empty line here. Please follow comment conventions per doc/coding-style.txt Section M2. There should be an empty line here per doc/coding-style.txt Section M1. Another empty line here (before and after if/while/do/for blocks) Regards, -Denis

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; Modified HACKING and man page to have more formation on what are the debugging options and how to enable them. --- HACKING | 10 ++++++++++ doc/ofonod.8 | 5 ++++- 2 files changed, 14 insertions(+), 1 deletions(-) diff --git a/HACKING b/HACKING index ae420aa..e825185 100644 --- a/HACKING +++ b/HACKING @@ -81,3 +81,13 @@ automatically includes this option. For production installations or distribution packaging it is important that the "--enable-maintainer-mode" option is NOT used. + +Note multiple arguments to -d can be specified, colon, comma or space +separated. The arguments are relative source code filenames for which +debugging output should be enabled; output shell-style globs are +accepted (e.g.: 'plugins/*:src/main.c'). + +Other debugging settings that can be toggled: + + - Environment variable OFONO_AT_DEBUG (set to 1): enable AT commands + debugging diff --git a/doc/ofonod.8 b/doc/ofonod.8 index 474d7fb..7bb908c 100644 --- a/doc/ofonod.8 +++ b/doc/ofonod.8 @@ -18,7 +18,10 @@ is used to manage \fID-Bus\fP permissions for oFono. .SH OPTIONS .TP .B --debug, -d -Enable debug information output. +Enable debug information output. Note multiple arguments to -d can be +specified, colon, comma or space separated. The arguments are relative +source code filenames for which debugging output should be enabled; +output shell-style globs are accepted (e.g.: "plugins/*:src/main.c"). .TP .B --nodetach, -n Don't run as daemon in background. -- 1.6.6.1

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; This adds a simple API to use for generating unique IDs for SMS messages. Note this adds missing #includes to src/smsutil.h; header files should included from any other .c or .h file. --- Makefile.am | 5 +- src/smsutil.c | 191 +++++++++++++++++++++++++++++++++++++++++++ src/smsutil.h | 87 ++++++++++++++++++++ unit/test-sms-msg-id.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 494 insertions(+), 1 deletions(-) create mode 100644 unit/test-sms-msg-id.c diff --git a/Makefile.am b/Makefile.am index e256841..5a371c2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -361,7 +361,7 @@ unit_objects = noinst_PROGRAMS = unit/test-common unit/test-util unit/test-idmap \ unit/test-sms unit/test-simutil \ unit/test-mux unit/test-caif \ - unit/test-stkutil + unit/test-stkutil unit/test-sms-msg-id unit_test_common_SOURCES = unit/test-common.c src/common.c unit_test_common_LDADD = @GLIB_LIBS@ @@ -400,6 +400,9 @@ unit_test_caif_SOURCES = unit/test-caif.c $(gatchat_sources) \ unit_test_caif_LDADD = @GLIB_LIBS@ unit_objects += $(unit_test_caif_OBJECTS) +unit_test_sms_msg_id_SOURCES = unit/test-sms-msg-id.c src/util.c src/smsutil.c src/storage.c +unit_test_sms_msg_id_LDADD = @GLIB_LIBS@ + noinst_PROGRAMS += gatchat/gsmdial gatchat/test-server gatchat/test-qcdm gatchat_gsmdial_SOURCES = gatchat/gsmdial.c $(gatchat_sources) diff --git a/src/smsutil.c b/src/smsutil.c index 6c2087a..f93ea5b 100644 --- a/src/smsutil.c +++ b/src/smsutil.c @@ -3982,3 +3982,194 @@ char *ussd_decode(int dcs, int len, const unsigned char *data) return utf8; } + +static +void sms_log_critical(const gchar *format, ...) +{ + va_list args; + + va_start(args, format); + g_logv("SMS-MSG-ID", G_LOG_LEVEL_CRITICAL | G_LOG_FLAG_FATAL, + format, args); + va_end(args); +} + +const GChecksumType SMS_MSG_ID_CHECKSUM = G_CHECKSUM_SHA256; + + +/* + * Internal function (also used for aiding in unit testing, hence not static) + */ +gsize __sms_msg_id_hash_length(void) +{ + return g_checksum_type_get_length(SMS_MSG_ID_CHECKSUM); +} + + +/** + * Initialize and reset a UUID's state + * + * \param msg_id Descriptor + */ +void sms_msg_id_init(struct sms_msg_id *msg_id) +{ + memset(msg_id->id, 0, sizeof(msg_id->id)); + memset(&msg_id->checksum, 0, sizeof(msg_id->checksum)); +} + + +/** + * Create a random UUID + * + * \param msg_id Descriptor + * + * \internal + * + * Crams the msg_id ID buffer with random 32-bit numbers; because we + * use a byte based buffer, we play the cast to guint32 trick (as that + * is what g_random_int() returns). + */ +void sms_msg_id_random(struct sms_msg_id *msg_id) +{ + guint32 *b; + unsigned cnt, top; + + /* Fail building if the size of the ID block is not a full + * multiple of a guint32's size. */ + BUILD_BUG_ON(sizeof(msg_id->id) % sizeof(b[0]) != 0); + + top = sizeof(msg_id->id) / sizeof(b[0]); + b = (void *) msg_id->id; + for (cnt = 0; cnt < top; cnt++) + b[cnt] = g_random_int(); + /* mark hash is initialized */ + msg_id->checksum = (void *) ~0; +} + + +/** + * Feed data to the hash for generation of a UUID + * + * Provides a data buffer to be added to the hash from which a UUID + * will be generated. When done providing buffers, call with a %NULL + * buffer to get the hash finalized and the UUID. + * + * \param msg_id Descriptor + * \param buf Pointer to data buffer. Call with %NULL to finalize the + * hash and generate the UUID. Note that after calling with %NULL + * you cannot call again with another buffer to generate another + * UUID without calling sms_msg_id_init() first. + * \param buf_size size of the @buf buffer. + * + * NOTE: + * - Will abort on OOM with an error message + * - Will abort if called with %NULL as an argument to \param buf + * without any non-NULL buffers were fed to in previous calls. + * - In case of going over an error path before closing the UUID with + * sms_msg_id_hash(%NULL), the hash has to be closed in order to + * avoid memory leaks -- thus call sms_msg_id_hash(%NULL) in the + * error path. + */ +void sms_msg_id_hash(struct sms_msg_id *msg_id, + const void *buf, size_t buf_size) +{ + gsize digest_size = __sms_msg_id_hash_length(); + unsigned char digest[digest_size]; + + if (msg_id->checksum == (void *) ~0) { + sms_log_critical("%s:%d: SW BUG: %s(!NULL) called " + "on a closed hash\n", + __FILE__, __LINE__, __func__); + return; + } + if (buf == NULL) { + if (msg_id->checksum == NULL) { + sms_log_critical("%s:%d: BUG: %s(NULL) called " + "without feeding data first\n", + __FILE__, __LINE__, __func__); + return; + } + g_checksum_get_digest(msg_id->checksum, + digest, &digest_size); + memcpy(msg_id->id, digest, + MIN(digest_size, sizeof(msg_id->id))); + if (digest_size < sizeof(msg_id->id)) + memset(msg_id->id + digest_size, 0, + sizeof(msg_id->id) - digest_size); + g_checksum_free(msg_id->checksum); + /* mark hash is initialized */ + msg_id->checksum = (void *) ~0; + } else { + /* Word has it g_checksum_new(), which uses + * g_slice_alloc() will abort() on allocation + * failure. However, is not really documented + * anywhere, so the extra check -- and we just abort + * on OOM. */ + if (msg_id->checksum == NULL) { + msg_id->checksum = g_checksum_new(SMS_MSG_ID_CHECKSUM); + if (msg_id->checksum == NULL) + sms_log_critical("%s:%d: OOM: can't allocate " + "checksum", + __FILE__, __LINE__); + } + g_checksum_update(msg_id->checksum, buf, buf_size); + } +} + + +/** + * Print a UUID to a string buffer + * + * Given a closed UUID, print it as a hexadecimal string to a provided + * buffer display (lowest nibble right). + * + * \param msg_id Descriptor + * \param strbuf String where to format the UUID + * \param strbuf_size Size of the string buffer. + * + * NOTE: @msg_id has to be closed, ie: sms_msg_id_random() or + * sms_msg_id_hash(NULL) were called on them. + */ +void sms_msg_id_printf(const struct sms_msg_id *msg_id, + char *strbuf, size_t strbuf_size) +{ + int cnt; + size_t written = 0; + + if (msg_id->checksum != (void *) ~0) { + sms_log_critical("%s:%d: BUG: %s() called on an open hash\n", + __FILE__, __LINE__, __func__); + return; + } + for (cnt = 0; cnt < SMS_MSG_ID_HASH_SIZE; cnt++) + written += snprintf(strbuf + written, strbuf_size - written, + "%02x", msg_id->id[cnt] & 0xff); +} + + +/** + * Print a UUID to a string buffer + * + * Given a closed UUID, print it as a hexadecimal string to a provided + * buffer display (lowest nibble right). + * + * \param msg_id Descriptor + * \param strbuf String where to format the UUID + * \param strbuf_size Size of the string buffer. + * + * NOTE: @msg_id has to be closed, ie: sms_msg_id_random() or + * sms_msg_id_hash(NULL) were called on them. + */ +void sms_msg_id_memcpy(void *buf, size_t buf_size, + const struct sms_msg_id *msg_id) +{ + size_t copy_size = MIN(buf_size, sizeof(msg_id->id)); + if (msg_id->checksum != (void *) ~0) { + sms_log_critical("%s:%d: BUG: %s() called on an open hash\n", + __FILE__, __LINE__, __func__); + return; + } + memmove(buf, msg_id->id, MIN(buf_size, sizeof(msg_id->id))); + if (copy_size < buf_size) + memset(buf + copy_size, 0, buf_size - copy_size); +} diff --git a/src/smsutil.h b/src/smsutil.h index 66ef6f8..fbe3c72 100644 --- a/src/smsutil.h +++ b/src/smsutil.h @@ -19,6 +19,12 @@ * */ +#include <time.h> +#include <glib/gtypes.h> +#include <glib/gslist.h> +#include <glib/ghash.h> +#include <glib/gchecksum.h> + #define CBS_MAX_GSM_CHARS 93 enum sms_type { @@ -539,3 +545,84 @@ GSList *cbs_optimize_ranges(GSList *ranges); gboolean cbs_topic_in_range(unsigned int topic, GSList *ranges); char *ussd_decode(int dcs, int len, const unsigned char *data); + +enum { + /* Note this has to be a multiple of sizeof(guint32); + * Compilation will assert-fail if this is not met. */ + SMS_MSG_ID_HASH_SIZE = 16, +}; + +/** + * Unique identifiers + * + * Generate unique identifiers to map data. Can be initialized with a + * random identifier or by creating a hash based on data contents. + * + * \NOTE Never access the struct's contents directly; use the + * sms_msg_id*() API + * + * Usage + * + * Declare and initialize: + * + * \code + * struct sms_msg_id msg_id; + * + * sms_msg_id_init(&msg_id); // initialize to known state + * \endcode + * + * Initialize with a random ID + * + * \code + * sms_msg_id_random(&msg_id); // Create a random unique ID + * \endcode + * + * or initialize by hashing data buffers: + * + * \code + * sms_msg_id_hash(&msg_id, buf1, buf1_size); + * sms_msg_id_hash(&msg_id, buf2, buf2_size); + * sms_msg_id_hash(&msg_id, buf3, buf3_size); + * ... + * sms_msg_id_hash(&msg_id, NULL, 0); + * \endcode + * + * Once the hash has been initialized, it can be accessed. Never + * access it before initializing the hash (it will trigger a software + * error and abort). + * + * Formating for printing: + * + * \code + * DECLARE_SMS_MSG_ID_STRBUF(buf); + * ... + * sms_msg_id_printf(&msg_id, buf, sizeof(buf)); + * \endcode + * + * Copying the hash data to a buffer + * + * \code + * sms_msg_id_memcpy(buf, buf_size, &msg_id); + * \endcode + * + * For generating another ID (with sms_msg_id_random() or + * sms_msg_id_hash()), sms_msg_id_init() has to be called. + */ + +struct sms_msg_id { + guint8 id[SMS_MSG_ID_HASH_SIZE]; + GChecksum *checksum; +}; + +#define DECLARE_SMS_MSG_ID_STRBUF(strbuf) \ + char strbuf[SMS_MSG_ID_HASH_SIZE * 2 + 1] + +void sms_msg_id_init(struct sms_msg_id *); +void sms_msg_id_random(struct sms_msg_id *); +void sms_msg_id_hash(struct sms_msg_id *, const void *, size_t); + +void sms_msg_id_printf(const struct sms_msg_id *, char *, size_t); +void sms_msg_id_memcpy(void *, size_t, const struct sms_msg_id *); + +/* for unit testing only */ +gsize __sms_msg_id_hash_length(void); diff --git a/unit/test-sms-msg-id.c b/unit/test-sms-msg-id.c new file mode 100644 index 0000000..f9dd5e2 --- /dev/null +++ b/unit/test-sms-msg-id.c @@ -0,0 +1,212 @@ +/* + * Exercise flows and corner cases of the SMS-MSG-ID API + * + * This unit attempts to prove the common code flow and corner cases + * of the SMS-MSG-ID API, as described by it's functions usage + * documentation. + * + * Relies on the fact that the SMS-MSG-ID code will call + * sms_log_critical() for every condition that warrants an abort. This + * will call g_logv() with a G_LOG_FLAG_FATAL flag, which can be + * intercepted at glib level by setting a fatal log handler + * [test_fatal_log()] with g_test_log_set_fatal_handler() and decide + * based on the return value if the program should abort (TRUE) or not + * (FALSE). Instead we set a flag to indicate the 'condition' has been + * detected. + */ + +#include <smsutil.h> +#include <stdio.h> +#include <string.h> +#include <glib/gtestutils.h> + +int fatal_error; + +gboolean test_fatal_log(const gchar *log_domain, + GLogLevelFlags log_level, + const gchar *message, + gpointer user_data) +{ + fatal_error = 1; + return FALSE; /* Avoid program abort */ +} + +int main(void) +{ + int fail = 0; + struct sms_msg_id msg_id1, msg_id2; + size_t hash_length = __sms_msg_id_hash_length(); + size_t id_length = SMS_MSG_ID_HASH_SIZE; + size_t cnt; + DECLARE_SMS_MSG_ID_STRBUF(str_id1); + DECLARE_SMS_MSG_ID_STRBUF(str_id2); + char blob1[SMS_MSG_ID_HASH_SIZE], + blob2[SMS_MSG_ID_HASH_SIZE], + blob_big[4 * hash_length]; + + printf("I: '**CRITICAL**' and 'aborting...' messages " + "should be ignored\n"); + + /* mask fatal errors so they don't crash, but just notes it + * happened @fatal_error -- this is called by + * sms_log_critical() -> g_logv() [as it uses the + * G_LOG_FLAG_FATAL indicator. */ + fatal_error = 0; + g_test_log_set_fatal_handler(test_fatal_log, NULL); + + sms_msg_id_init(&msg_id1); + sms_msg_id_init(&msg_id2); + + /* should trap: hash not initialized */ + sms_msg_id_printf(&msg_id1, str_id1, sizeof(str_id1)); + if (fatal_error == 1) { + printf("I: %d: sms_msg_id_printf() doesn't work on " + "unitialized hash. OK\n", __LINE__); + fatal_error = 0; + } else { + fprintf(stderr, "E: %d: sms_msg_id_printf() didn't reject an " + "unitialized hash. FAILURE\n", __LINE__); + fail = 1; + } + sms_msg_id_memcpy(blob1, sizeof(blob1), &msg_id1); + if (fatal_error == 1) { + printf("I: %d: sms_msg_id_memcpy() doesn't work on " + "unitialized hash. OK\n", __LINE__); + fatal_error = 0; + } else { + fprintf(stderr, "E: %d: sms_msg_id_memcpy() didn't reject an " + "unitialized hash. FAILURE.\n", __LINE__); + fail = 1; + } + + /* Make a random initialization */ + sms_msg_id_random(&msg_id1); + sms_msg_id_random(&msg_id2); + + /* Printf */ + sms_msg_id_printf(&msg_id1, str_id1, sizeof(str_id1)); + sms_msg_id_printf(&msg_id2, str_id2, sizeof(str_id2)); + /* should be different */ + if (strcmp(str_id1, str_id2)) + printf("I: %d: strings for 1 & 2 are different, OK\n", + __LINE__); + else { + fprintf(stderr, "E: %d: strings for 1 & 2 match. FAILURE.\n", + __LINE__); + fail = 1; + } + printf("I: %d: str1 %s, str2 %s\n", __LINE__, str_id1, str_id2); + + /* Reinitialize to test hash -- we feed the strings as binary + * data */ + sms_msg_id_init(&msg_id1); + /* Test sms_msg_id_hash(NULL) fails if no data was fed */ + sms_msg_id_hash(&msg_id1, NULL, 0); + if (fatal_error == 1) { + printf("I: %d: sms_msg_id_hash() rejects a non-fed " + "hash. OK\n", __LINE__); + fatal_error = 0; + } else { + fprintf(stderr, "E: %d: sms_msg_id_hash() didn't reject a " + "non-fed hash. FAILURE.\n", __LINE__); + fail = 1; + } + sms_msg_id_hash(&msg_id1, str_id1, sizeof(str_id1)); + sms_msg_id_hash(&msg_id1, str_id2, sizeof(str_id2)); + sms_msg_id_hash(&msg_id1, NULL, 0); + /* Should fail if we try to add more */ + sms_msg_id_hash(&msg_id1, str_id1, sizeof(str_id1)); + if (fatal_error == 1) { + printf("I: %d: sms_msg_id_hash() rejects a closed " + "hash. OK\n", __LINE__); + fatal_error = 0; + } else { + fprintf(stderr, "E: %d: sms_msg_id_hash() didn't reject a " + "closed hash. FAILURE.\n", __LINE__); + fail = 1; + } + /* ... or close */ + sms_msg_id_hash(&msg_id1, NULL, 0); + if (fatal_error == 1) { + printf("I: %d: sms_msg_id_hash() rejects closing a closed " + "hash. OK\n", __LINE__); + fatal_error = 0; + } else { + fprintf(stderr, "E: %d: sms_msg_id_hash() didn't reject " + " closing a closed hash. FAILURE.\n", __LINE__); + fail = 1; + } + + sms_msg_id_init(&msg_id2); + sms_msg_id_hash(&msg_id2, str_id1, sizeof(str_id1)); + sms_msg_id_hash(&msg_id2, str_id2, sizeof(str_id2)); + sms_msg_id_hash(&msg_id2, NULL, 0); + + /* extract the blobs, they should match as we generated the + * hashes from identical buckets */ + sms_msg_id_memcpy(blob1, sizeof(blob1), &msg_id1); + sms_msg_id_memcpy(blob2, sizeof(blob2), &msg_id2); + if (memcmp(blob1, blob2, sizeof(blob1))) { + fprintf(stderr, "E: %d: hashes don't match on identical feed. " + "FAILURE.\n", __LINE__); + fail = 1; + } else + printf("I: %d: hashes match on identical feed. " + "OK.\n", __LINE__); + sms_msg_id_printf(&msg_id1, str_id1, sizeof(str_id1)); + sms_msg_id_printf(&msg_id2, str_id2, sizeof(str_id2)); + /* should be the same */ + if (!strcmp(str_id1, str_id2)) + printf("I: %d: strings for 1 & 2 match. OK\n", + __LINE__); + else { + fprintf(stderr, "E: %d: strings for 1 & 2 differ. FAILURE.\n", + __LINE__); + fail = 1; + } + printf("I: %d: str1 %s, str2 %s\n", __LINE__, str_id1, str_id2); + + /* if the blob is bigger than the hash, the rest should be + * filled with zeroes */ + memset(blob_big, 0xff, sizeof(blob_big)); + sms_msg_id_memcpy(blob_big, sizeof(blob_big), &msg_id1); + for (cnt = hash_length; cnt < sizeof(blob_big); cnt++) + if (blob_big[cnt] != 0) { + fprintf(stderr, "E: %d: blob @ #%u not zero. " + "FAILURE.\n", __LINE__, cnt); + fail = 1; + break; + } + if (cnt >= sizeof(blob_big)) + printf("I: %d: remainder blob is zero. OK\n", __LINE__); + + /* if the blob is smaller than the hash, the remainder should + * not be touched */ + sms_msg_id_init(&msg_id1); + sms_msg_id_random(&msg_id1); + + sms_msg_id_init(&msg_id2); + sms_msg_id_random(&msg_id2); + + sms_msg_id_memcpy(blob1, sizeof(blob1), &msg_id1); + /* Note we cut by four here! */ + memcpy(blob1, blob2, sizeof(blob1)); + sms_msg_id_memcpy(blob2, id_length / 4, &msg_id2); + + for (cnt = id_length / 4; cnt < id_length; cnt++) + if (blob1[cnt] != blob2[cnt]) { + fprintf(stderr, "E: %d: blob1 @ #%u doesn't match " + "blob2. FAILURE.\n", __LINE__, cnt); + fail = 1; + break; + } + if (cnt >= id_length) + printf("I: %d: remainder blob is untouched. OK\n", + __LINE__); + + if (fail) + fprintf(stderr, "E: FAILURE.\n"); + else + printf("I: OK\n"); + return fail; +} -- 1.6.6.1

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; Introduce DECLARE_SMS_ADDR_STR(), which declares a string variable of the right size for passing to sms_assembly_decode_address(). This way we detach each client having to have the knowledge of what the right size is, leaving that decission to the infrastructure provider. Updated couple of sites in smsutil.c to use it vs a raw declaration. --- src/smsutil.c | 4 ++-- src/smsutil.h | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/smsutil.c b/src/smsutil.c index f93ea5b..3f743ef 100644 --- a/src/smsutil.c +++ b/src/smsutil.c @@ -2315,7 +2315,7 @@ static void sms_assembly_load(struct sms_assembly *assembly, const struct dirent *dir) { struct sms_address addr; - char straddr[25]; + DECLARE_SMS_ADDR_STR(straddr); guint16 ref; guint8 max; guint8 seq; @@ -2392,7 +2392,7 @@ static gboolean sms_assembly_store(struct sms_assembly *assembly, { unsigned char buf[177]; int len; - char straddr[25]; + DECLARE_SMS_ADDR_STR(straddr); if (!assembly->imsi) return FALSE; diff --git a/src/smsutil.h b/src/smsutil.h index fbe3c72..59297a0 100644 --- a/src/smsutil.h +++ b/src/smsutil.h @@ -438,6 +438,12 @@ gboolean sms_decode_unpacked_stk_pdu(const unsigned char *pdu, int len, gboolean sms_encode(const struct sms *in, int *len, int *tpdu_len, unsigned char *pdu); +/* + * Length is based on the address being 12 hex characters plus a + * terminating NUL char. See sms_assembly_extract_address(). + */ +#define DECLARE_SMS_ADDR_STR(a) char a[25] + gboolean sms_decode_address_field(const unsigned char *pdu, int len, int *offset, gboolean sc, struct sms_address *out); -- 1.6.6.1

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; The new name better reflects the function's purpose. We need to export it, as for generating unique message naming (for persistence and D-Bus object naming), we'll be using the address. --- src/smsutil.c | 7 +++---- src/smsutil.h | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/smsutil.c b/src/smsutil.c index 3f743ef..969c1b7 100644 --- a/src/smsutil.c +++ b/src/smsutil.c @@ -2294,8 +2294,7 @@ static gboolean sms_assembly_extract_address(const char *straddr, return sms_decode_address_field(pdu, len, &offset, FALSE, out); } -static gboolean sms_assembly_encode_address(const struct sms_address *in, - char *straddr) +gboolean sms_address_to_hex_string(const struct sms_address *in, char *straddr) { unsigned char pdu[12]; int offset = 0; @@ -2397,7 +2396,7 @@ static gboolean sms_assembly_store(struct sms_assembly *assembly, if (!assembly->imsi) return FALSE; - if (sms_assembly_encode_address(&node->addr, straddr) == FALSE) + if (sms_address_to_hex_string(&node->addr, straddr) == FALSE) return FALSE; len = sms_serialize(buf, sms); @@ -2420,7 +2419,7 @@ static void sms_assembly_backup_free(struct sms_assembly *assembly, if (!assembly->imsi) return; - if (sms_assembly_encode_address(&node->addr, straddr) == FALSE) + if (sms_address_to_hex_string(&node->addr, straddr) == FALSE) return; for (seq = 0; seq < node->max_fragments; seq++) { diff --git a/src/smsutil.h b/src/smsutil.h index 59297a0..28b5604 100644 --- a/src/smsutil.h +++ b/src/smsutil.h @@ -506,6 +506,8 @@ GSList *sms_assembly_add_fragment(struct sms_assembly *assembly, const struct sms_address *addr, guint16 ref, guint8 max, guint8 seq); void sms_assembly_expire(struct sms_assembly *assembly, time_t before); +gboolean sms_address_to_hex_string(const struct sms_address *in, + char *straddr); struct status_report_assembly *status_report_assembly_new(const char *imsi); void status_report_assembly_free(struct status_report_assembly *assembly); -- 1.6.6.1

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; This patch removes the sequential SMS message identification gig and replaces it with a 16-bit hash cookie based off message contents. FIXME: [incomplete] need to figure out how to do so that identical messages sent to the same number also have a different ID. Note that in order to feed the reference 16-bit ID to set_ref_and_to(), the UUID has to be computed as early as possible. --- src/ofono.h | 4 +- src/sms.c | 68 +++++++++++++++++++++++++++++++++++++--------------------- src/stk.c | 4 +- 3 files changed, 47 insertions(+), 29 deletions(-) diff --git a/src/ofono.h b/src/ofono.h index aaa01d9..1b72381 100644 --- a/src/ofono.h +++ b/src/ofono.h @@ -185,8 +185,8 @@ enum ofono_sms_submit_flag { typedef void (*ofono_sms_txq_submit_cb_t)(gboolean ok, void *data); -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list, - unsigned int flags, +struct tx_queue_entry * __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list, + unsigned int flags, unsigned ref, ofono_sms_txq_submit_cb_t cb, void *data, ofono_destroy_func destroy); diff --git a/src/sms.c b/src/sms.c index 71e24c1..49f3e54 100644 --- a/src/sms.c +++ b/src/sms.c @@ -55,7 +55,6 @@ struct ofono_sms { DBusMessage *pending; struct ofono_phone_number sca; struct sms_assembly *assembly; - unsigned int next_msg_id; guint ref; GQueue *txq; gint tx_source; @@ -590,7 +589,12 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, struct ofono_modem *modem; unsigned int flags; unsigned int msg_id; - + struct sms_msg_id sms_msg_id; + DECLARE_SMS_MSG_ID_STRBUF(msg_id_str); + struct sms_address receiver; + DECLARE_SMS_ADDR_STR(receiver_str); + struct tx_queue_entry *sms_msg; + if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to, DBUS_TYPE_STRING, &text, DBUS_TYPE_INVALID)) @@ -598,6 +602,16 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, if (valid_phone_number_format(to) == FALSE) return __ofono_error_invalid_format(msg); + /* + * Instead of using the telephone number/address we got from + * D-Bus, we do the reverse formatting, so we get something + * that has been normalized--this is used later and we do it + * here to simplify error handling. + */ + sms_address_from_string(&receiver, to); + if (sms_address_to_hex_string(&receiver, receiver_str) + == FALSE) + return __ofono_error_failed(msg); msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset, sms->use_delivery_reports); @@ -605,22 +619,21 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, if (!msg_list) return __ofono_error_invalid_format(msg); - set_ref_and_to(msg_list, sms->ref, ref_offset, to); - DBG("ref: %d, offset: %d", sms->ref, ref_offset); - - if (ref_offset != 0) { - if (sms->ref == 65536) - sms->ref = 1; - else - sms->ref = sms->ref + 1; - } + sms_msg_id_init(&sms_msg_id); + sms_msg_id_hash(&sms_msg_id, text, strlen(text)); + sms_msg_id_hash(&sms_msg_id, receiver_str, strlen(receiver_str)); + sms_msg_id_hash(&sms_msg_id, NULL, 0); + sms_msg_id_memcpy(&msg_id, sizeof(msg_id), &sms_msg_id); + sms_msg_id_printf(&sms_msg_id, msg_id_str, sizeof(msg_id_str)); + set_ref_and_to(msg_list, msg_id, ref_offset, to); + DBG("ref: %d, offset: %d", msg_id, ref_offset); flags = OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY; flags |= OFONO_SMS_SUBMIT_FLAG_RETRY; if (sms->use_delivery_reports) flags |= OFONO_SMS_SUBMIT_FLAG_REQUEST_SR; - msg_id = __ofono_sms_txq_submit(sms, msg_list, flags, send_message_cb, + sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id, send_message_cb, dbus_message_ref(msg), send_message_destroy); @@ -676,6 +689,9 @@ static void dispatch_text_message(struct ofono_sms *sms, struct tm remote; struct tm local; const char *str = buf; + struct sms_msg_id sms_msg_id; + unsigned int id; + DECLARE_SMS_MSG_ID_STRBUF(msg_id_str); if (!message) return; @@ -717,11 +733,18 @@ static void dispatch_text_message(struct ofono_sms *sms, g_dbus_send_message(conn, signal); - if (cls != SMS_CLASS_0) { - __ofono_history_sms_received(modem, sms->next_msg_id, str, + sms_msg_id_init(&sms_msg_id); + sms_msg_id_hash(&sms_msg_id, message, strlen(message)); + sms_msg_id_hash(&sms_msg_id, addr->address, strlen(addr->address)); + /* FIXME: add RX time? */ + sms_msg_id_hash(&sms_msg_id, NULL, 0); + sms_msg_id_memcpy(&id, sizeof(id), &sms_msg_id); + sms_msg_id_printf(&sms_msg_id, msg_id_str, sizeof(msg_id_str)); + ofono_debug("SMS RX: msg id is %s\n", msg_id_str); + + if (cls != SMS_CLASS_0) + __ofono_history_sms_received(modem, id, str, &remote, &local, message); - sms->next_msg_id += 1; - } } static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list) @@ -1104,8 +1127,6 @@ static void sms_remove(struct ofono_atom *atom) if (sms->settings) { g_key_file_set_integer(sms->settings, SETTINGS_GROUP, - "NextMessageId", sms->next_msg_id); - g_key_file_set_integer(sms->settings, SETTINGS_GROUP, "NextReference", sms->ref); g_key_file_set_boolean(sms->settings, SETTINGS_GROUP, "UseDeliveryReports", @@ -1201,9 +1222,6 @@ static void sms_load_settings(struct ofono_sms *sms, const char *imsi) sms->imsi = g_strdup(imsi); - sms->next_msg_id = g_key_file_get_integer(sms->settings, SETTINGS_GROUP, - "NextMessageId", NULL); - sms->ref = g_key_file_get_integer(sms->settings, SETTINGS_GROUP, "NextReference", NULL); if (sms->ref >= 65536) @@ -1306,8 +1324,8 @@ void *ofono_sms_get_data(struct ofono_sms *sms) return sms->driver_data; } -unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list, - unsigned int flags, +struct tx_queue_entry * __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list, + unsigned int flags, unsigned msg_id, ofono_sms_txq_submit_cb_t cb, void *data, ofono_destroy_func destroy) { @@ -1320,7 +1338,7 @@ unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list, sizeof(entry->receiver)); } - entry->msg_id = sms->next_msg_id++; + entry->msg_id = msg_id; entry->flags = flags; entry->cb = cb; entry->data = data; @@ -1331,5 +1349,5 @@ unsigned int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list, if (g_queue_get_length(sms->txq) == 1) sms->tx_source = g_timeout_add(0, tx_next, sms); - return entry->msg_id; + return entry; } diff --git a/src/stk.c b/src/stk.c index 556dc68..400b033 100644 --- a/src/stk.c +++ b/src/stk.c @@ -338,8 +338,8 @@ static gboolean handle_command_send_sms(const struct stk_command *cmd, msg_list.data = (void *) &cmd->send_sms.gsm_sms; msg_list.next = NULL; - - __ofono_sms_txq_submit(sms, &msg_list, 0, send_sms_submit_cb, +#warning FIXME: fix msg_id, missing + __ofono_sms_txq_submit(sms, &msg_list, 0, 0, send_sms_submit_cb, stk->sms_submit_req, g_free); stk->cancel_cmd = send_sms_cancel; -- 1.6.6.1

Hi Inaky, On 07/23/2010 03:59 PM, Inaky Perez-Gonzalez wrote: Actually keep string to, I like that better. The returned type should be 'object' (the object path of the new message) not 'string'. Try to use tabs and not spaces for indentation. array{object} here, and using Messages is better. oFono API is full CamelCase, so this should be SmsMessage. Please follow the conventions for Object Path, e.g. see doc/voicecall-api.txt Rename this to State and include the possible state values please. Regards, -Denis

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; --- src/sms.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/src/sms.c b/src/sms.c index 49f3e54..b53f880 100644 --- a/src/sms.c +++ b/src/sms.c @@ -887,6 +887,15 @@ static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming) g_slist_free(l); } + +/* + * Handle a delivery/status report has been received for an SMS + * apparently sent from here. + * + * We need to find the message in the SMS manager's + * waiting-for-acknoledge queue (sms->tx_wfaq) and remove it. As well, + * fill out history for it. + */ static void handle_sms_status_report(struct ofono_sms *sms, const struct sms *incoming) { -- 1.6.6.1

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; Introduce a destructor function to encapsulate all the release steps for this data type, as more are to be added later. --- src/sms.c | 25 +++++++++++++++++++------ 1 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/sms.c b/src/sms.c index 85353ff..5f5df04 100644 --- a/src/sms.c +++ b/src/sms.c @@ -405,6 +405,23 @@ static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg, return __ofono_error_invalid_args(msg); } + +/* + * Destroy/release the contents of a 'struct tx_queue_entry' + * + * This releases resources allocated *inside* @entry and @entry + * itself. + */ +static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused) +{ + struct tx_queue_entry *entry = _entry; + + if (entry->destroy) + entry->destroy(entry->data); + g_free(entry->pdus); + g_free(entry); +} + static void tx_finished(const struct ofono_error *error, int mr, void *data) { struct ofono_sms *sms = data; @@ -465,11 +482,7 @@ next_q: time(NULL), hs); } - if (entry->destroy) - entry->destroy(entry->data); - - g_free(entry->pdus); - g_free(entry); + tx_queue_entry_destroy_free(entry, NULL); if (g_queue_peek_head(sms->txq)) { DBG("Scheduling next"); @@ -1129,7 +1142,7 @@ static void sms_remove(struct ofono_atom *atom) } if (sms->txq) { - g_queue_foreach(sms->txq, (GFunc)g_free, NULL); + g_queue_foreach(sms->txq, tx_queue_entry_destroy_free, NULL); g_queue_free(sms->txq); sms->txq = NULL; } -- 1.6.6.1

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; This will ease adding later on other fields that are D-Bus specific and needed to export SMS messages in TX transit over D-Bus. --- src/sms.c | 21 +++++++++++++++++++-- 1 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/sms.c b/src/sms.c index 5f5df04..7bde919 100644 --- a/src/sms.c +++ b/src/sms.c @@ -90,6 +90,17 @@ struct tx_queue_entry { ofono_destroy_func destroy; }; +/* + * Encapsulate information needed to export an SMS message over D-Bus. + * + * @dbus_path: path of the object in the D-Bus + * @dbus_msg: message this originated at + */ +struct sms_msg_dbus_data { + char *dbus_path; + DBusMessage *dbus_msg; +}; + static const char *sms_bearer_to_string(int bearer) { switch (bearer) { @@ -560,7 +571,8 @@ static struct tx_queue_entry *tx_queue_entry_new(GSList *msg_list) static void send_message_cb(gboolean ok, void *data) { DBusConnection *conn = ofono_dbus_get_connection(); - DBusMessage *msg = data; + struct sms_msg_dbus_data *dbus_data = data; + DBusMessage *msg = dbus_data->dbus_msg; DBusMessage *reply; if (ok) @@ -573,9 +585,11 @@ static void send_message_cb(gboolean ok, void *data) static void send_message_destroy(void *data) { - DBusMessage *msg = data; + struct sms_msg_dbus_data *dbus_data = data; + DBusMessage *msg = dbus_data->dbus_msg; dbus_message_unref(msg); + g_free(dbus_data); } /* @@ -590,6 +604,9 @@ static void send_message_destroy(void *data) * is created by tx_queue_entry_new() and g_queue_push_tail() * appends that entry to the SMS transmit queue. Then the tx_next() * function is scheduled to run to process the queue. + * + * @sms is the main SMS driver struct, @entry and @msg_list represent + * the current message being processed. */ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, void *data) -- 1.6.6.1

On Tue, 2010-07-27 at 10:08 -0700, Denis Kenzior wrote: First it serves as transitional storage -- later in the commit sequences the dbus_msg is removed as it is, as you say, unneeded once all the code is made sync. If at the end only one member is left (dbus_msg) In our discussions to plan this stuff you said you wanted the D-Bus unique name to contain the UUID and the number of PDUs (/modemX/UUID_PDUs). That means I need to access the SMS object if I want to generate the name on the fly. I need a backpointer for that. Even if we remove the _PDUs requirement: To generate on the fly whenever is needed we need to have access to who is the 'parent' in: - the destructor function, which is not going to be easy - the PropertyChanged signal generator which means that instead of storing a name I have to store, for example the 'struct ofono_sms' that is the parent, get its name, figure out how to pull the name of the SMS object from a UUID *and* then generate a name *and* do the actual work. That is not effective and adds more code. Generating the name once and storing it is the simplest solution. If you want it out of the structure that encapsulates it, well I can do that as a final follow up patch once dbus_msg is no longer needed. It's syntactic sugar to enforce encapsulation. Yep, it is -- this should be a separate doc commit. Thanks for catching it.

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; A WSR (Waiting for Status Report) state definition is added to the state transtition machine; as well, a queue (ofono_sms->tx_wfaq) where messages waiting for an status report are queued. When the message's delivery is acknowledged, the message is removed from the queue and the message's status machine is updated, which can trigger things such as D-Bus signals. --- src/sms.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 files changed, 61 insertions(+), 9 deletions(-) diff --git a/src/sms.c b/src/sms.c index de6eb7f..1075f33 100644 --- a/src/sms.c +++ b/src/sms.c @@ -67,6 +67,11 @@ const char *ofono_sms_tx_state_to_string(enum ofono_sms_tx_state status) } +/** + * @tx_wsrq: Waiting-for-Status-Report queue; messages in this queue + * have been delivered but are waiting to be acknoledged by the + * network. + */ struct ofono_sms { int flags; DBusMessage *pending; @@ -74,6 +79,7 @@ struct ofono_sms { struct sms_assembly *assembly; guint ref; GQueue *txq; + GQueue *tx_wsrq; gint tx_source; struct ofono_message_waiting *mw; unsigned int mw_watch; @@ -487,10 +493,19 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, case OFONO_SMS_TX_ST_QUEUED: ofono_sms_tx_state_check( file, line, entry, state_old, state_new, + 1 << OFONO_SMS_TX_ST_WSR + | 1 << OFONO_SMS_TX_ST_DONE + | 1 << OFONO_SMS_TX_ST_CANCELING); + g_queue_remove(entry->sms_mgr->txq, entry); + break; + case OFONO_SMS_TX_ST_WSR: + ofono_sms_tx_state_check( + file, line, entry, state_old, state_new, 1 << OFONO_SMS_TX_ST_DONE | 1 << OFONO_SMS_TX_ST_CANCELING - | 1 << OFONO_SMS_TX_ST_FAILED); - g_queue_remove(entry->sms_mgr->txq, entry); + | 1 << OFONO_SMS_TX_ST_FAILED + | 1 << OFONO_SMS_TX_ST_EXPIRED); + g_queue_remove(entry->sms_mgr->tx_wsrq, entry); break; case OFONO_SMS_TX_ST_CANCELING: ofono_sms_tx_state_check( @@ -522,8 +537,8 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, * Destroy/release the contents of a 'struct tx_queue_entry' * * This releases resources allocated *inside* @entry and @entry - * itself. We blindly remove from the submission queue, in case it is - * still in any. We could do it with a 'checked' state change to + * itself. We blindly remove from both submission queues, in case it + * is still in any. We could do it with a 'checked' state change to * INVALID if we modify that function. */ static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused) @@ -534,6 +549,7 @@ static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused) entry->destroy(entry->data); g_free(entry->pdus); g_queue_remove(entry->sms_mgr->txq, entry); + g_queue_remove(entry->sms_mgr->tx_wsrq, entry); entry->state = __OFONO_SMS_TX_ST_INVALID; g_free(entry); } @@ -586,6 +602,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data) next_q: entry = g_queue_peek_head(sms->txq); +#warning FIXME: cb should not be called on WSR? if (entry->cb) entry->cb(ok, entry->data); @@ -599,12 +616,16 @@ next_q: __ofono_history_sms_send_status(modem, entry->msg_id, time(NULL), hs); } - if (ok) + if (ok && entry->flags & OFONO_SMS_SUBMIT_FLAG_REQUEST_SR) { + ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_WSR); + g_queue_push_tail(sms->tx_wsrq, entry); + } else if (ok && !(entry->flags & OFONO_SMS_SUBMIT_FLAG_REQUEST_SR)) { ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_DONE); - else + tx_queue_entry_destroy_free(entry, NULL); + } else { ofono_sms_tx_state_set(entry, OFONO_SMS_TX_ST_FAILED); - - tx_queue_entry_destroy_free(entry, NULL); + tx_queue_entry_destroy_free(entry, NULL); + } if (g_queue_peek_head(sms->txq)) { DBG("Scheduling next"); @@ -1030,11 +1051,31 @@ static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming) /* + * This function is a g_queue_foreach() functor called by + * handle_sms_status_report() to destroy the message that matches the + * reported MSG-ID in the SMS manager's list of messages waiting for + * acknowledgement. + */ +static void __sms_find_destroy_by_msg_id(gpointer _sms_msg, gpointer _msg_id) +{ + unsigned msg_id = (unsigned) _msg_id; + struct tx_queue_entry *sms_msg = _sms_msg; + + if (sms_msg->msg_id != msg_id) + return; + ofono_debug("SMS: ACKED %p msg_id match %x", sms_msg, msg_id); + g_queue_remove(sms_msg->sms_mgr->tx_wsrq, sms_msg); + ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_DONE); + tx_queue_entry_destroy_free(sms_msg, NULL); +} + + +/* * Handle a delivery/status report has been received for an SMS * apparently sent from here. * * We need to find the message in the SMS manager's - * waiting-for-acknoledge queue (sms->tx_wfaq) and remove it. As well, + * waiting-for-status report queue (sms->tx_wsrq) and remove it. As well, * fill out history for it. */ static void handle_sms_status_report(struct ofono_sms *sms, @@ -1048,6 +1089,9 @@ static void handle_sms_status_report(struct ofono_sms *sms, &delivered) == FALSE) return; + g_queue_foreach(sms->tx_wsrq, __sms_find_destroy_by_msg_id, + (void *) msg_id); + __ofono_history_sms_send_status(modem, msg_id, time(NULL), delivered ? OFONO_HISTORY_SMS_STATUS_DELIVERED : OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED); @@ -1275,6 +1319,13 @@ static void sms_remove(struct ofono_atom *atom) sms->txq = NULL; } + if (sms->tx_wsrq) { + g_queue_foreach(sms->tx_wsrq, + tx_queue_entry_destroy_free, NULL); + g_queue_free(sms->tx_wsrq); + sms->tx_wsrq = NULL; + } + if (sms->settings) { g_key_file_set_integer(sms->settings, SETTINGS_GROUP, "NextReference", sms->ref); @@ -1329,6 +1380,7 @@ struct ofono_sms *ofono_sms_create(struct ofono_modem *modem, sms->sca.type = 129; sms->ref = 1; sms->txq = g_queue_new(); + sms->tx_wsrq = g_queue_new(); sms->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_SMS, sms_remove, sms); -- 1.6.6.1

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; Splits sms_send_message() into a D-Bus front end and an internal API: - sms_msg_send(), a full C interface for SMS sending - dbus_sms_msg_send(): adapts sms_msg_send() to be a D-Bus call, exporting the object on the bus, returning its name and setting up all the callbacks and D-Bus specific data needed in a 'struct sms_msg_dbus_data'. The call is synchronous. This allows internal code to use the same infrastructure as D-Bus clients to send SMS messages. --- src/sms.c | 192 +++++++++++++++++++++++++++++++++++++++------------------ src/smsutil.h | 12 ++++ 2 files changed, 145 insertions(+), 59 deletions(-) diff --git a/src/sms.c b/src/sms.c index 668a645..4569cf3 100644 --- a/src/sms.c +++ b/src/sms.c @@ -41,6 +41,10 @@ #define SMS_MANAGER_FLAG_CACHED 0x1 +/* D-Bus interface name for SMS messages (not for the manager!) */ +static +const char SMS_MSG_INTERFACE[] = "org.ofono.SMSMessage"; + #define SETTINGS_STORE "sms" #define SETTINGS_GROUP "Settings" @@ -123,11 +127,9 @@ struct tx_queue_entry { * Encapsulate information needed to export an SMS message over D-Bus. * * @dbus_path: path of the object in the D-Bus - * @dbus_msg: message this originated at */ struct sms_msg_dbus_data { char *dbus_path; - DBusMessage *dbus_msg; }; static const char *sms_bearer_to_string(int bearer) @@ -536,6 +538,26 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, /* + * D-Bus SMS Message interface + * + * NOTE: the sms_msg_{methods,signals} tables should be const, but + * then g_dbus_register_interface() type warns. + */ + +static +GDBusMethodTable sms_msg_methods[] = { + { } +}; + + +static +GDBusSignalTable sms_msg_signals[] = { + { "PropertyChanged", "sv" }, + { } +}; + + +/* * Destroy/release the contents of a 'struct tx_queue_entry' * * This releases resources allocated *inside* @entry and @entry @@ -554,6 +576,7 @@ static void tx_queue_entry_destroy_free(gpointer _entry, gpointer unused) g_queue_remove(entry->sms_mgr->tx_wsrq, entry); entry->state = __OFONO_SMS_TX_ST_INVALID; g_free(entry); + ofono_debug("%s():%d: sms_msg %p freed", __func__, __LINE__, entry); } static void tx_finished(const struct ofono_error *error, int mr, void *data) @@ -694,55 +717,35 @@ static struct tx_queue_entry *tx_queue_entry_new(GSList *msg_list) DBG("pdu_len: %d, tpdu_len: %d", pdu->pdu_len, pdu->tpdu_len); } - return entry; } -static void send_message_stch_cb(void *data, enum ofono_sms_tx_state new_state) -{ - DBusConnection *conn = ofono_dbus_get_connection(); - struct sms_msg_dbus_data *dbus_data = data; - DBusMessage *msg = dbus_data->dbus_msg; - DBusMessage *reply; - gboolean ok; - /* - * We care about only the final states - * (DONE/CANCELLED/FAILED/EXPIRED), the rest are just - * ignored. - */ - if (new_state == OFONO_SMS_TX_ST_DONE) - ok = TRUE; - else if (new_state == OFONO_SMS_TX_ST_CANCELLED - || new_state == OFONO_SMS_TX_ST_FAILED - || new_state == OFONO_SMS_TX_ST_EXPIRED) - ok = FALSE; - else - return; - - if (ok) - reply = dbus_message_new_method_return(msg); - else - reply = __ofono_error_failed(msg); - - g_dbus_send_message(conn, reply); -} - -static void send_message_destroy(void *data) +static void sms_msg_dbus_destroy(void *_dbus_data) { - struct sms_msg_dbus_data *dbus_data = data; - DBusMessage *msg = dbus_data->dbus_msg; + struct sms_msg_dbus_data *dbus_data = _dbus_data; - dbus_message_unref(msg); + if (dbus_data == NULL) + return; + g_dbus_unregister_interface(ofono_dbus_get_connection(), + dbus_data->dbus_path, SMS_MSG_INTERFACE); + g_free(dbus_data->dbus_path); + memset(dbus_data, 0, sizeof(*dbus_data)); g_free(dbus_data); } + /* - * Pre-process a SMS text message and deliver it [D-Bus SendMessage()] + * Pre-process a SMS text message and deliver it * * @conn: D-Bus connection * @msg: message data (telephone number and text) * @data: SMS object to use for transmision + * @send_flags: flags that manipulate how the message is sent. + * @send_cb: function called when the message is sent + * @destroy_cb: function called when the message structure is + * being released + * @priv: pointer to pass to the @data and @send_cb functions * * An alphabet is chosen for the text and it (might be) segmented in * fragments by sms_text_prepare() into @msg_list. A queue list @entry @@ -753,12 +756,12 @@ static void send_message_destroy(void *data) * @sms is the main SMS driver struct, @entry and @msg_list represent * the current message being processed. */ -static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, - void *data) +struct tx_queue_entry *sms_msg_send( + struct ofono_sms *sms, const char *to, const char *text, + enum sms_msg_send_flags send_flags, + ofono_sms_msg_stch_cb_t stch_cb, + void (*destroy_cb)(void *), void *priv) { - struct ofono_sms *sms = data; - const char *to; - const char *text; GSList *msg_list; int ref_offset; struct ofono_modem *modem; @@ -770,13 +773,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, DECLARE_SMS_ADDR_STR(receiver_str); struct tx_queue_entry *sms_msg; - if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to, - DBUS_TYPE_STRING, &text, - DBUS_TYPE_INVALID)) - return __ofono_error_invalid_args(msg); - if (valid_phone_number_format(to) == FALSE) - return __ofono_error_invalid_format(msg); + return NULL; /* * Instead of using the telephone number/address we got from * D-Bus, we do the reverse formatting, so we get something @@ -786,13 +784,13 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, sms_address_from_string(&receiver, to); if (sms_address_to_hex_string(&receiver, receiver_str) == FALSE) - return __ofono_error_failed(msg); + return NULL; msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset, - sms->use_delivery_reports); + sms->use_delivery_reports); if (!msg_list) - return __ofono_error_invalid_format(msg); + return NULL; sms_msg_id_init(&sms_msg_id); sms_msg_id_hash(&sms_msg_id, text, strlen(text)); @@ -803,23 +801,100 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, set_ref_and_to(msg_list, msg_id, ref_offset, to); DBG("ref: %d, offset: %d", msg_id, ref_offset); - flags = OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY; + if (send_flags & SMS_MSG_SEND_HISTORY) + flags = OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY; flags |= OFONO_SMS_SUBMIT_FLAG_RETRY; if (sms->use_delivery_reports) flags |= OFONO_SMS_SUBMIT_FLAG_REQUEST_SR; sms_msg = __ofono_sms_txq_submit(sms, msg_list, flags, msg_id, - send_message_stch_cb, - dbus_message_ref(msg), - send_message_destroy); + stch_cb, priv, destroy_cb); g_slist_foreach(msg_list, (GFunc)g_free, NULL); g_slist_free(msg_list); modem = __ofono_atom_get_modem(sms->atom); - __ofono_history_sms_send_pending(modem, msg_id, to, time(NULL), text); - return NULL; + return sms_msg; +} + + +/* + * D-Bus: Send a SMS text message + * + * @conn: D-Bus connection + * @msg: message data (telephone number and text) + * @data: SMS object to use for transmision + * + * Unwraps the arguments and sends the request to sms_msg_send() + */ +static DBusMessage *dbus_sms_msg_send(DBusConnection *conn, DBusMessage *msg, + void *data) +{ + struct ofono_sms *sms = data; + const char *to; + const char *text; + struct tx_queue_entry *sms_msg; + DBusMessage *reply; + struct sms_msg_dbus_data *dbus_data; + const char *sms_path = __ofono_atom_get_path(sms->atom); + + if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to, + DBUS_TYPE_STRING, &text, + DBUS_TYPE_INVALID)) + return __ofono_error_invalid_args(msg); + + dbus_data = g_new0(struct sms_msg_dbus_data, 1); + sms_msg = sms_msg_send(sms, to, text, SMS_MSG_SEND_HISTORY, + NULL, + sms_msg_dbus_destroy, + dbus_data); + if (sms_msg == NULL) + goto error_sms_msg_send; + /* We can only fill out these data when we have the + * information we need off @sms_msg; this is all released by @ + * sms_msg_dbus_destroy() */ + dbus_data->dbus_path = g_strdup_printf(SMS_MSG_NAME_FMT, sms_path, + sms_msg->msg_id, + sms_msg->num_pdus); + + if (!g_dbus_register_interface(ofono_dbus_get_connection(), + dbus_data->dbus_path, SMS_MSG_INTERFACE, + sms_msg_methods, sms_msg_signals, + NULL, sms_msg, NULL)) { + ofono_error("%s():%d: %s: Could not create %s interface", + __func__, __LINE__, dbus_data->dbus_path, + SMS_MSG_INTERFACE); + goto error_dbus_register_interface; + } + ofono_debug("%s():%d: sms %p @ %s: MSG registered", + __func__, __LINE__, sms_msg, dbus_data->dbus_path); + + reply = dbus_message_new_method_return(msg); + if (!reply) + goto error_dbus_new_method_return; + dbus_message_append_args(reply, + DBUS_TYPE_STRING, &dbus_data->dbus_path, + DBUS_TYPE_INVALID); + return reply; + + +error_dbus_new_method_return: + g_dbus_unregister_interface(ofono_dbus_get_connection(), + dbus_data->dbus_path, SMS_MSG_INTERFACE); +error_dbus_register_interface: + g_free(dbus_data->dbus_path); + /* + * Note we don't want the destructor called, as we do it's + * steps manually here when falling through. Kind of layering + * violation, but it is the cleaner way as things are + * currently laid out. + */ + sms_msg->destroy = NULL; + tx_queue_entry_destroy_free(sms_msg, NULL); +error_sms_msg_send: + g_free(dbus_data); + return __ofono_error_invalid_format(msg); } static GDBusMethodTable sms_manager_methods[] = { @@ -827,8 +902,7 @@ static GDBusMethodTable sms_manager_methods[] = { G_DBUS_METHOD_FLAG_ASYNC }, { "SetProperty", "sv", "", sms_set_property, G_DBUS_METHOD_FLAG_ASYNC }, - { "SendMessage", "ss", "", sms_send_message, - G_DBUS_METHOD_FLAG_ASYNC }, + { "SendMessage", "ss", "", dbus_sms_msg_send, 0 }, { } }; diff --git a/src/smsutil.h b/src/smsutil.h index 28b5604..20b24f0 100644 --- a/src/smsutil.h +++ b/src/smsutil.h @@ -215,6 +215,12 @@ enum cbs_geo_scope { CBS_GEO_SCOPE_CELL_NORMAL }; +/* Flags to sms_msg_send() */ +enum sms_msg_send_flags { + /* Record/dont this message to the history database */ + SMS_MSG_SEND_HISTORY = 0x01, +}; + struct sms_address { enum sms_number_type number_type; enum sms_numbering_plan numbering_plan; @@ -634,3 +640,9 @@ void sms_msg_id_memcpy(void *, size_t, const struct sms_msg_id *); /* for unit testing only */ gsize __sms_msg_id_hash_length(void); + +/* + * UNIQUE-ID_#PDUS -- note we use underscores as to generate a name + * suitable to be a D-Bus name. + */ +#define SMS_MSG_NAME_FMT "%s/%04x_%i" -- 1.6.6.1

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez(a)intel.com&gt; This introduces the ability to cancel a pending SMS message, accessible via an internal API and over a D-Bus wrapper. Sending a note to the network to cancel an in-transit message is not yet implemented. Note the test case code requires follow up commits that propagate the message's state changes over D-Bus. --- src/sms.c | 63 +++++++++++++++++ src/smsutil.h | 15 ++++ test/test-sms-msg-cancel | 173 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 251 insertions(+), 0 deletions(-) create mode 100755 test/test-sms-msg-cancel diff --git a/src/sms.c b/src/sms.c index c0b2e00..97c9eab 100644 --- a/src/sms.c +++ b/src/sms.c @@ -538,6 +538,24 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, /* + * Note that the D-Bus specific cleanups are taken care by the + * sms_msg_dbus_destroy() callback passed in dbus_sms_msg_send(). + */ +static DBusMessage *dbus_sms_msg_cancel( + DBusConnection * conn, DBusMessage *msg, void *data) +{ + gboolean do_network_cancel = FALSE; + struct tx_queue_entry *sms_msg = data; + if (!dbus_message_get_args(msg, NULL, + DBUS_TYPE_BOOLEAN, &do_network_cancel)) + return __ofono_error_invalid_args(msg); + sms_msg_cancel(sms_msg, + do_network_cancel ? SMS_MSG_CANCEL_IN_NETWORK : 0); + return dbus_message_new_method_return(msg); +} + + +/* * D-Bus SMS Message interface * * NOTE: the sms_msg_{methods,signals} tables should be const, but @@ -546,6 +564,8 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, static GDBusMethodTable sms_msg_methods[] = { + { "Cancel", DBUS_TYPE_BOOLEAN_AS_STRING, "", + dbus_sms_msg_cancel, 0 }, { } }; @@ -587,6 +607,11 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data) gboolean ok = error->type == OFONO_ERROR_TYPE_NO_ERROR; DBG("tx_finished"); + /* + * Queue is empty? Messages might have been cancelled. + */ + if (entry == NULL) + return; if (ok == FALSE) { if (!(entry->flags & OFONO_SMS_SUBMIT_FLAG_RETRY)) { @@ -819,6 +844,44 @@ struct tx_queue_entry *sms_msg_send( } +/** + * Cancel a pending SMS message + * + * @sms_msg: message to cancel + * @flags: modifies for the action + * + * This function cancels a message that is pending or being + * actively transmitted. Note that after this function returns, the + * @sms_msg handle is no longer valid. + * + * \internal + * + * There is no need to cancel the calling of tx_next() by + * g_timeout_add() scheduled in sms_msg_send(). The rationale behind + * this is that the tx_next() function is scheduled to go over the + * list of messages in the @sms object, so it might have been + * scheduled for other messages also rather than just for this one + * @sms_msg. By the time it gets to run, it might see the list empty + * or see other messages, but @sms_msg won't be there. + */ +void sms_msg_cancel(struct tx_queue_entry *sms_msg, + enum sms_msg_cancel_flags flags) +{ + struct ofono_sms *sms = sms_msg->sms_mgr; + DBG("%s (%p)\n", __func__, sms_msg); + if (sms_msg->state == OFONO_SMS_TX_ST_QUEUED) + g_queue_remove(sms->txq, sms_msg); + else if (sms_msg->state == OFONO_SMS_TX_ST_WSR) + g_queue_remove(sms->tx_wsrq, sms_msg); + ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_CANCELING); + if (flags & SMS_MSG_CANCEL_IN_NETWORK) + ofono_warn("%s(): SMS_MSG_CANCEL_IN_NETWORK support " + "not yet implemented\n", __func__); + ofono_sms_tx_state_set(sms_msg, OFONO_SMS_TX_ST_CANCELLED); + tx_queue_entry_destroy_free(sms_msg, NULL); +} + + /* * Send a PropertyChange signal when the state changes * diff --git a/src/smsutil.h b/src/smsutil.h index 20b24f0..fce4278 100644 --- a/src/smsutil.h +++ b/src/smsutil.h @@ -560,6 +560,21 @@ gboolean cbs_topic_in_range(unsigned int topic, GSList *ranges); char *ussd_decode(int dcs, int len, const unsigned char *data); +/* + * SMS Message API + */ + +enum sms_msg_cancel_flags { + /** + * The cancelled message will be reported to the network so it + * tries to cancel it if in transit. + */ + SMS_MSG_CANCEL_IN_NETWORK = 0x1, +}; + +struct tx_queue_entry; +void sms_msg_cancel(struct tx_queue_entry *sms_msg, enum sms_msg_cancel_flags); + enum { /* Note this has to be a multiple of sizeof(guint32); * Compilation will assert-fail if this is not met. */ diff --git a/test/test-sms-msg-cancel b/test/test-sms-msg-cancel new file mode 100755 index 0000000..24cc7e2 --- /dev/null +++ b/test/test-sms-msg-cancel @@ -0,0 +1,173 @@ +#!/usr/bin/python +# +# Sends a message and inmediately cancels it once the state moves to +# "queued"; fails if the message doesn't move to the CANCELLED state. +# +# Arguments are: DEST-PHONE-NUMBER MSG-TEXT BOOLEAN +# +# BOOLEAN: request delivery confirmation / no +# +# Notes: accessing the globals result and reason using 'globals' +# + +import gobject +import sys +import dbus +import dbus.mainloop.glib + +dbus.mainloop.glib.DBusGMainLoop(set_as_default=True) + +class sms_msg_cancel_test: + number = "" + text = "" + wsr = "" + target_state = "" + + result = "" + reason = "" + + sms_msg_path = "" + + # FIXME + def __init__(self, number, text, wsr): + self.number = number + self.text = text + self.wsr = wsr + self.mainloop = gobject.MainLoop() + + self.dbus = dbus.SystemBus() + ofono_manager = dbus.Interface(self.dbus.get_object('org.ofono', '/'), + 'org.ofono.Manager') + path = ofono_manager.GetProperties()["Modems"][0] + self.sms_manager = dbus.Interface(self.dbus.get_object('org.ofono', path), + 'org.ofono.SmsManager') + self.sms_manager.SetProperty("UseDeliveryReports", dbus.Boolean(int(self.wsr))) + + # Connect the signal *before* creating the message, otherwise we miss + # the transition - this also means we can't use sms_msg.connect_to_signal() + self.dbus.add_signal_receiver(self.property_changed, bus_name="org.ofono", + signal_name = "PropertyChanged", + path_keyword="path", + interface_keyword="interface") + self.result = "FAILED" + self.reason = "didn't see CANCELING state" + + # Cancel the message and update test state + def msg_cancel(self): + print "I: %s[%s]: SMS message: canceling because " \ + "target state was reached" \ + % (self.sms_msg_path, self.target_state) + sms_msg = dbus.Interface(self.dbus.get_object('org.ofono', self.sms_msg_path), + 'org.ofono.SMSMessage') + try: + sms_msg.Cancel(0) + except dbus.DBusException as dbus_ex: + if dbus_ex.get_dbus_name() != 'org.freedesktop.DBus.Error.UnknownMethod': + pass + # Object has been cancelled already--this is + # why we get the D-Bus exception, so we can + # consider this a win + print "I: %s[%s]: SMS message: ok, D-Bus object already removed" \ + % (self.sms_msg_path, self.target_state) + self.result = "SUCCESS" + self.reason = "object dissapeared from the bus, meaning it was cancelled" + self.mainloop.quit() + return + + # Run the actual test FIXME + def run(self, target_state): + self.target_state = target_state + + self.result = "FAILED" + self.reason = "didn't see %s state to allow cancelling" \ + % target_state + + print "I: [%s]: starting test" % self.target_state + self.sms_msg_path = self.sms_manager.SendMessage(number, text) + print "I: %s[%s]: SMS message: D-Bus object created" \ + % (self.sms_msg_path, self.target_state) + + # wait for property changed signals, property_changed() will be called + # when there is a transition and the thing cancelled. + gobject.timeout_add(20000, self.timeout_fail) + # If the target state to cancel on is QUEUED, schedule a + # cancellation inmediately -- note we need to schedule it in + # the mainloop to ensure property_changed() signals are + # properly propagated. + if target_state == "queued" \ + or target_state == "canceling" \ + or target_state == "cancelled": + gobject.idle_add(self.msg_cancel) + self.mainloop.run() + if self.result == "SUCCESS": + letter = "I" + result = 0 + else: + letter = "E" + result = 1 + print "%s: %s[%s]: %s: %s" % (letter, self.sms_msg_path, + self.target_state, + self.result, self.reason) + return result + + def timeout_fail(self): + self.result = "FAILED" + self.reason = "%s (timedout)" % self.reason + self.mainloop.quit() + # Don't rearm the timeout ... doesn't really matter as we + # quit the mainloop. + return 0 + + + # When the message's state (the one we created) switches to QUEUED, cancel + def property_changed(self, property_name, property_value, + path, interface): + if interface != "org.ofono.SMSMessage": + return + if self.sms_msg_path != path: + return + if property_name != "TXState": + return + print "I: %s[%s]: SMS message: transitioning to %s state" \ + % (self.sms_msg_path, self.target_state, property_value) + if property_value == "canceling": + print "I: %s[%s]: SMS message: ok, saw canceling state" \ + % (self.sms_msg_path, self.target_state) + self.result = "FAIL" + self.reason = "didn't see the CANCELLED state" + if property_value == "cancelled": + print "I: %s[%s]: SMS message: ok, saw cancelled state, quiting" \ + % (self.sms_msg_path, self.target_state) + self.result = "SUCCESS" + self.reason = "saw CANCELLED state" + self.mainloop.quit() + # If we get to the state that is the target state for + # the test, cancel the message -- we need to do this + # after evaluating propery_value or we might enter + # into conflicts (as self.msg_cancel() will change + # self.{result,reason} if it turns out the operation + # is complete). + if self.target_state == property_value: + # Cancel the message + self.msg_cancel() + + +# Send the message +if len(sys.argv) == 4: + wsr = int(sys.argv[3]) +elif len(sys.argv) == 3: + wsr = 0 +else: + raise NameError("E: Bad number of arguments (expected NUMBER TEXT [BOOLEAN])") + +number = sys.argv[1] +text = sys.argv[2] +global test + +test = sms_msg_cancel_test(number, text, wsr) + +test.run('queued') +test.run('wsr') +test.run('canceling') +test.run('cancelled') +test.run('done') -- 1.6.6.1

Hi Inaky, Empty line here Empty line here Empty line here And one more here Also, you might want to think about what other properties would be useful here. E.g. perhaps queue time, submission time, To address, contents? No double empty lines allowed. Why exactly is this method ASYNC? Regards, -Denis