SMS D-Bus support and misc small patches
cover SMS D-Bus support and misc small patches
From: Inaky Perez-Gonzalez <[email protected]>
This (3rd? 4th?) version of the patchset builds the D-Bus support on top of the new _txq_submit() callback mechanism.
Note there are still a couple of opens that need discussion:
- the message ID is generated based on the contents of the message -- thus, the current way doesn't work. We need the caller to _txq_submit() to generate it. It's been left out of the STK stc.c:handle_command_send_sms() because I am not sure what is the right way to do it -- need feedback on that.
- The generation of the SMS message ID based on contents still has
shortcomings: if we submit two messages with the same content and destination number, the ID is the same [sms.c:sms_msg_send()]. What other factor would make sense to add? time?
The following changes since commit 94344e967b4cd3edd65aa5254ef4b4f5dd037e69:
Denis Kenzior (1):
TODO: Major updates to STK related tasks
are available in the git repository at:
git://gitorious.org/~inakypg/ofono/ofono-inakypg.git master
Patches follow for reviewing convenience.
Inaky Perez-Gonzalez (20):
bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
write_file: make transaction-safe
manpage: explain debugging options to -d
SMS: introduce message ID API
introduce DECLARE_SMS_ADDR_STR()
_assembly_encode_address: export and rename
SMS: implement SHA256-based message IDs [incomplete]
sms: document the org.ofono.SMSMessage D-Bus interface
SMS: document handle_sms_status_report()
sms_text_prepare: document @use_delivery_reports
SMS: rename create_tx_queue_entry() to tx_queue_entry_new() struct tx_queue_entry: add a destructor
SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'
SMS: introduce bare state machine and transitions
SMS: introduce Wait-for-Status-Report state and infrastructure
SMS: introduce a state change callback for TX messages
SMS: export outgoing messages over D-Bus
SMS: send D-Bus SMS-MSG::PropertyChanged signals when message changes status
SMS: introduce sms_msg_cancel and its D-Bus wrapper
SMS: Implement D-Bus SMS-MSG::GetProperties
HACKING | 10 + Makefile.am | 5 +- doc/ofonod.8 | 5 +- doc/sms-api.txt | 49 ++++- src/bug.h | 50 ++++ src/ofono.h | 42 +++- src/sms.c | 598 ++++++++++++++++++++++++++++++++++------ src/smsutil.c | 206 ++++++++++++++- src/smsutil.h | 122 ++++++++ src/stk.c | 24 ++- src/storage.c | 42 ++- test/test-sms-msg-cancel | 173 ++++++++++++ test/test-sms-msg-state-change | 24 ++ unit/test-sms-msg-id.c | 212 ++++++++++++++ 14 files changed, 1449 insertions(+), 113 deletions(-) create mode 100644 src/bug.h create mode 100755 test/test-sms-msg-cancel create mode 100755 test/test-sms-msg-state-change create mode 100644 unit/test-sms-msg-id.c
PATCH 1/20 bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
From: Inaky Perez-Gonzalez <[email protected]>
These have been stolen from the Linux kernel source; come pretty handy to make build-time consistency checks and thus avoid run-time surprises.
--- src/bug.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/smsutil.c | 1 + 2 files changed, 51 insertions(+), 0 deletions(-) create mode 100644 src/bug.h
src/bug.h new file +50 -0
@@ -0,0 +1,50 @@ +/* +* +* oFono - Open Source Telephony +* +* Copyright (C) 2008-2010 Intel Corporation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +* +*/ + +/* +* Build time consistency checks +* +* Copied from the Linux kernel, linux/include/linux/kernel.h +* v2.6.35-rc; this file has no explicit license header, thus is +* covered by linux/COPYING which licenses it as GPL v2 only. +*/ + +/* Force a compilation error if condition is true */ +#define BUILD_BUG_ON (condition) ((void)BUILD_BUG_ON_ZERO(condition)) + +/* Force a compilation error if condition is constant and true */ +#define MAYBE_BUILD_BUG_ON (cond) ((void)sizeof(char[1 - 2 * !!(cond)])) + +/* Force a compilation error if a constant expression is not a power of 2 */ +#define BUILD_BUG_ON_NOT_POWER_OF_2 (n) \ + BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0)) + +/* Force a compilation error if condition is true, but also produce a +result (of value 0 and type size_t), so the expression can be used +e.g. in a structure initializer (or where-ever else comma expressions +aren't permitted). */ +#define BUILD_BUG_ON_ZERO (e) (sizeof(struct { int:-!!(e); })) +#define BUILD_BUG_ON_NULL (e) ((void *)sizeof(struct { int:-!!(e); })) + +/* +* End of code copied from linux/include/linux/kernel.h +* v2.6.35-rc. +*/
src/smsutil.c +1 -0
@@ -37,6 +37,7 @@#include "util.h" #include "storage.h" #include "smsutil.h" +#include "bug.h" #define uninitialized_var (x) x = x
Signature · 1 line
1.6.6.1
Re: [patch 01/20] bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
Hi Inaky,
Quoted 11 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
These have been stolen from the Linux kernel source; come pretty handy to make build-time consistency checks and thus avoid run-time surprises.
--- src/bug.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/smsutil.c | 1 + 2 files changed, 51 insertions(+), 0 deletions(-) create mode 100644 src/bug.h
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
src/smsutil.c line 40
Quoted 9 lines
src/smsutil.c +1
-0@@ -37,6 +37,7 @@#include "util.h" #include "storage.h" #include "smsutil.h" +#include "bug.h"
Why are you including this without actually using it?
src/smsutil.c line 43
Quoted 3 lines
src/smsutil.c +0
-0#define uninitialized_var (x) x = x
Regards,
-Denis
Re: [patch 01/20] bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
Quoted 15 lines · Denis Kenzior
Hi Inaky,
Quoted 11 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
These have been stolen from the Linux kernel source; come pretty handy to make build-time consistency checks and thus avoid run-time surprises.
--- src/bug.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/smsutil.c | 1 + 2 files changed, 51 insertions(+), 0 deletions(-) create mode 100644 src/bug.h
Quoted 1 line
First of all, please refer to
doc/coding-style.txtSection M5.
Ops, miss, reworked them in the whole patchset
Quoted 9 lines
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
another FAIL. However, I am hitting another one in missing ofono/types.h, even after having pulled the tip as of today. Is that one known? Anyway, fixing this one.
Quoted 11 lines
src/smsutil.c line 40
Quoted 9 lines
src/smsutil.c +1
-0@@ -37,6 +37,7 @@#include "util.h" #include "storage.h" #include "smsutil.h" +#include "bug.h" Why are you including this without actually using it?
It's used in a follow commit -- I mispliced that, sorry. Reworking it for the next submission.
Re: [patch 01/20] bug.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
Hi Inaky,
Quoted 10 lines
Quoted 9 lines
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
Quoted 4 lines
another FAIL. However, I am hitting another one in missing
ofono/types.h, even after having pulled the tip as of today. Is that one known? Anyway, fixing this one.
Make distcheck passes fine on $HEAD for me.
Regards,
-Denis
PATCH 2/20 write_file: make transaction-safe
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
write_file(), as written wasn't transaction-safe; a crash bewtween a file being open and the buffer being written before a safe close would leave the file with a set of undetermined contents.
Modified to the file is written to a temporary file name; once completed, it is renamed to the final name. This way, a crash in the middle doesn't leave half-baked files.
--- src/storage.c | 42 +++++++++++++++++++++++++++++++----------- 1 files changed, 31 insertions(+), 11 deletions(-)
src/storage.c +31 -11
@@ -98,11 +98,21 @@ ssize_t read_file(unsigned char *buffer, size_t len,return r; } +/* +* Write a buffer to a file in a transactionally safe form +* +* Given a buffer, write it to a file named after +* @path_fmt+args. However, to make sure the file contents are +* consistent (ie: a crash right after opening or during write() +* doesn't leave a file half baked), the contents are written to a +* file with a temporary name and when closed, it is renamed to the +* specified name (@path_fmt+args). +*/ ssize_t write_file (const unsigned char *buffer,size_t len,mode_t mode,const char *path_fmt, ...) {va_list ap; -char *path; +char *tmp_path, *path;ssize_t r;int fd;
@@ -110,26 +120,36 @@ ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, path =g_strdup_vprintf (path_fmt, ap);va_end (ap); -if (create_dirs (path, mode |S_IXUSR ) !=0 ) { -g_free (path); -return -1 ; - } + tmp_path =g_strdup_printf ("%s.XXXXXX.tmp" , path); - fd =TFR (open (path,O_WRONLY |O_CREAT |O_TRUNC , mode)); -if (fd ==-1 ) { -g_free (path); -return -1 ; - } + r =-1 ; +if (create_dirs (path, mode |S_IXUSR ) !=0 ) + goto error_create_dirs; + fd =TFR (g_mkstemp_full (tmp_path,O_WRONLY |O_CREAT |O_TRUNC , mode)); +if (fd ==-1 ) + goto error_mkstemp_full; r =TFR (write (fd, buffer, len));TFR (close (fd));if (r != (ssize_t ) len) { -unlink (path); r =-1 ; + goto error_write; } +/* Now that the file contents are written, rename to the real +* file name; this way we are uniquely sure that the whole +* thing is there. */ +unlink (path); +/* conserve @r's value from 'write' */ +if (link (tmp_path, path) ==-1 ) + r =-1 ; +error_write: +unlink (tmp_path); +error_mkstemp_full: +error_create_dirs: +g_free (tmp_path);g_free (path);return r; }
Signature · 1 line
1.6.6.1
Re: [patch 02/20] write_file: make transaction-safe
Hi Inaky,
Quoted 48 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
write_file(), as written wasn't transaction-safe; a crash bewtween a file being open and the buffer being written before a safe close would leave the file with a set of undetermined contents.Modified to the file is written to a temporary file name; once completed, it is renamed to the final name. This way, a crash in the middle doesn't leave half-baked files.
--- src/storage.c | 42 +++++++++++++++++++++++++++++++----------- 1 files changed, 31 insertions(+), 11 deletions(-)src/storage.c +11
-5@@ -98,11 +98,21 @@ ssize_t read_file(unsigned char *buffer, size_t len,return r; } +/* +* Write a buffer to a file in a transactionally safe form +* +* Given a buffer, write it to a file named after +* @path_fmt+args. However, to make sure the file contents are +* consistent (ie: a crash right after opening or during write() +* doesn't leave a file half baked), the contents are written to a +* file with a temporary name and when closed, it is renamed to the +* specified name (@path_fmt+args). +*/ ssize_t write_file (const unsigned char *buffer,size_t len,mode_t mode,const char *path_fmt, ...) {va_list ap; -char *path; +char *tmp_path, *path;ssize_t r;int fd;@@ -110,26 +120,36 @@ ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, path =g_strdup_vprintf (path_fmt, ap);va_end (ap); -if (create_dirs (path, mode |S_IXUSR ) !=0 ) { -g_free (path); -return -1 ; - }
src/storage.c ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, · line 127
Quoted 10 lines
src/storage.c +4
-5+ tmp_path =g_strdup_printf ("%s.XXXXXX.tmp" , path); - fd =TFR (open (path,O_WRONLY |O_CREAT |O_TRUNC , mode)); -if (fd ==-1 ) { -g_free (path); -return -1 ; - } + r =-1 ; +if (create_dirs (path, mode |S_IXUSR ) !=0 ) + goto error_create_dirs;
Please do me a favor and add an empty line here.
Quoted 7 lines
src/storage.c +3
-0+ fd =TFR (g_mkstemp_full (tmp_path,O_WRONLY |O_CREAT |O_TRUNC , mode)); +if (fd ==-1 ) + goto error_mkstemp_full; r =TFR (write (fd, buffer, len));TFR (close (fd));
src/storage.c ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, · line 143
Quoted 10 lines
src/storage.c +4
-1if (r != (ssize_t ) len) { -unlink (path); r =-1 ; + goto error_write; } +/* Now that the file contents are written, rename to the real +* file name; this way we are uniquely sure that the whole +* thing is there. */
Please follow comment conventions per doc/coding-style.txt Section M2.
src/storage.c ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, · line 144
Quoted 1 line
src/storage.c +1
-0+unlink (path);
There should be an empty line here per doc/coding-style.txt Section M1.
src/storage.c ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, · line 147
Quoted 3 lines
src/storage.c +3
-0+/* conserve @r's value from 'write' */ +if (link (tmp_path, path) ==-1 ) + r =-1 ;
Another empty line here (before and after if/while/do/for blocks)
src/storage.c ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, · line 155
Quoted 8 lines
src/storage.c +5
-0+error_write: +unlink (tmp_path); +error_mkstemp_full: +error_create_dirs: +g_free (tmp_path);g_free (path);return r; }
Regards,
-Denis
Re: [patch 02/20] write_file: make transaction-safe
Quoted 12 lines · Denis Kenzior
Hi Inaky,
Quoted 9 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
write_file(), as written wasn't transaction-safe; a crash bewtween a file being open and the buffer being written before a safe close would leave the file with a set of undetermined contents.Modified to the file is written to a temporary file name; once completed, it is renamed to the final name. This way, a crash in the middle doesn't leave half-baked files.
all fixed, thanks
PATCH 3/20 manpage: explain debugging options to -d
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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(-)
HACKING +10 -0
@@ -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
doc/ofonod.8 +4 -1
@@ -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.
Signature · 1 line
1.6.6.1
Re: [patch 03/20] manpage: explain debugging options to -d
Hi Inaky,
Quoted 9 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
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(-)
This one has been applied, thanks.
Regards,
-Denis
PATCH 4/20 SMS: introduce message ID API
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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
Makefile.am +4 -1
@@ -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)
src/smsutil.c +191 -0
194 lines not on this page · raw message
src/smsutil.h +87 -0
96 lines not on this page · raw message
unit/test-sms-msg-id.c new file +212 -0
212 lines not on this page · raw message
Signature · 1 line
1.6.6.1
Re: [patch 04/20] SMS: introduce message ID API
Hi Inaky,
Quoted 5 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
This adds a simple API to use for generating unique IDs for SMS messages.
Quoted 7 lines
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 ++++++++++++++++++++++++++++++++++++++++++++++++
Any time you change files in multiple directories, send multiple patches for each directory. E.g. src/smsutil.[ch] in patch 1, Makefile.am and unit/test-sms-msg-id.c in patch 2.
Quoted 36 lines
4 files changed, 494 insertions(+), 1 deletions(-) create mode 100644 unit/test-sms-msg-id.cMakefile.am +4
-1@@ -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)src/smsutil.c +2
-0@@ -3982,3 +3982,194 @@ char *ussd_decode(int dcs, int len, const unsigned char *data)return utf8; } + +static
src/smsutil.c char *ussd_decode(int dcs, int len, const unsigned char *data) · line 3996
Quoted 10 lines
src/smsutil.c +10
-0+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); +} +
Please refrain from doing this inside utility code. There should be no printf / logs in smsutil.c
src/smsutil.c char *ussd_decode(int dcs, int len, const unsigned char *data) · line 4008
Quoted 12 lines
src/smsutil.c +12
-0+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 ); +} + +
What is up with the double empty lines?
Quoted 29 lines
src/smsutil.c +29
-0+/** +* 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
src/smsutil.c char *ussd_decode(int dcs, int len, const unsigned char *data) · line 4047
Quoted 10 lines
src/smsutil.c +10
-0+* 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 ; +}
I really question the utility of this function. We should always be able to generate a message id based on the contents.
Quoted 118 lines
src/smsutil.c +118
-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) +{
src/smsutil.c char *ussd_decode(int dcs, int len, const unsigned char *data) · line 4175
Quoted 10 lines
src/smsutil.c +10
-0+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); +}
Exposing all this API as public leads to lots of totally unnecessary code. The philosophy should be the opposite, e.g. exposing as little as possible to the end user.
The API needs to be shrunk down to maybe 2-3 simple functions and most of the extra pedantic checks removed.
Quoted 92 lines
src/smsutil.h +77
-0@@ -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 93enum 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]
src/smsutil.h GSList *cbs_optimize_ranges(GSList *ranges); · line 628
Quoted 10 lines
src/smsutil.h +10
-0+ +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 );
I think this is just way too complicated ...
I suggest something like:
struct sms_uuid {
uint8_t id[hash_size];
};// oFono is not multithreaded, so static return is fine const char *sms_uuid_to_string(const struct ofono_uuid *uuid); int sms_uuid_from_sms_list(GSList *list,
struct ofono_uuid *out_uuid);
Regards,
-Denis
PATCH 5/20 introduce DECLARE_SMS_ADDR_STR()
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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(-)
src/smsutil.c +2 -2
@@ -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;
src/smsutil.h +6 -0
@@ -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);
Signature · 1 line
1.6.6.1
Re: [patch 05/20] introduce DECLARE_SMS_ADDR_STR()
Hi Inaky,
Quoted 8 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
Introduce
DECLARE_SMS_ADDR_STR(), which declares a string variable of the right size for passing tosms_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 insmsutil.cto use it vs a raw declaration.
Quoted 5 lines
--- src/smsutil.c | 4 ++-- src/smsutil.h | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-)
This one has been applied, thanks.
Regards,
-Denis
PATCH 6/20 _assembly_encode_address: export and rename
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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(-)
src/smsutil.c +3 -4
@@ -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++) {
src/smsutil.h +2 -0
@@ -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);
Signature · 1 line
1.6.6.1
Re: [patch 06/20] _assembly_encode_address: export and rename
Hi Inaky,
Quoted 4 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
The new name better reflects the function's purpose.
Quoted 8 lines
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(-)
Applied with a very small tweak to the commit header. Thanks.
Regards,
-Denis
PATCH 7/20 SMS: implement SHA256-based message IDs [incomplete]
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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(-)
src/ofono.h +2 -2
@@ -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);
src/sms.c +43 -25
139 lines not on this page · raw message
src/stk.c +2 -2
@@ -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;
Signature · 1 line
1.6.6.1
Re: [patch 07/20] SMS: implement SHA256-based message IDs [incomplete]
Hi Inaky,
src/ofono.h enum ofono_sms_submit_flag { · line 191
Quoted 6 lines
src/ofono.h +2
-2-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);
I disagree with this, you're modifying an ofono internal API function to return a structure which is opaque and cannot be manipulated. I'd rather see you returning a struct sms_uuid...
src/sms.c static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, · line 614
Quoted 10 lines
src/sms.c +10
-0+/* +* 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);
Please avoid mixing tabs and spaces for indentation. And in this case it is more readale to break up the function than the equals line. E.g.
if (sms_address_to_hex_string(&receiver,
receiver_str == FALSE) return __ofono_error_failed(msg);
Regards,
-Denis
Re: [patch 07/20] SMS: implement SHA256-based message IDs [incomplete]
Quoted 12 lines · Denis Kenzior
Hi Inaky,
src/ofono.h enum ofono_sms_submit_flag { · line 191
Quoted 6 lines
src/ofono.h +2
-2-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);I disagree with this, you're modifying an ofono internal API function to return a structure which is opaque and cannot be manipulated. I'd rather see you returning a struct
sms_uuid...
I need a handle to operate on. The D-Bus wrappers need something they can use to dispose of the structure (call tx_queue_entry_destroy() on the error path) and to pass to sms_msg_cancel(). The structure is not manipulated (except for accessing the UUID and #PDUs, which should probably be coded out to a helper).
This could be solved by having at the entry point of those functions or wrappers of a lookup function that looks for the UUID in the list of messages, but that would be adding more code and wasting time for no benefit. I disagree on adding unnecessary steps.
Quoted 14 lines
src/sms.c static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, · line 614
Quoted 10 lines
src/sms.c +10
-0+/* +* 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);Please avoid mixing tabs and spaces for indentation. And in this case it is more readale to break up the function than the equals line. E.g.
Quoted 3 lines
if (sms_address_to_hex_string(&receiver,
receiver_str == FALSE) return __ofono_error_failed(msg);
This is truly a matter of personal opinion; I for one cannot see how this is more readable than the other form :)
Re: [patch 07/20] SMS: implement SHA256-based message IDs [incomplete]
Quoted 20 lines · Inaky Perez-Gonzalez
Quoted 12 lines · Denis Kenzior
Hi Inaky,
src/ofono.h enum ofono_sms_submit_flag { · line 191
Quoted 6 lines
src/ofono.h +2
-2-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);I disagree with this, you're modifying an ofono internal API function to return a structure which is opaque and cannot be manipulated. I'd rather see you returning a struct
sms_uuid...I need a handle to operate on. The D-Bus wrappers need something they can use to dispose of the structure (call
tx_queue_entry_destroy()on the error path) and to pass tosms_msg_cancel(). The structure is not manipulated (except for accessing the UUID and #PDUs, which should probably be coded out to a helper).
Quoted 5 lines
This could be solved by having at the entry point of those functions or wrappers of a lookup function that looks for the UUID in the list of messages, but that would be adding more code and wasting time for no benefit. I disagree on adding unnecessary steps.
I need to look at this in detail. Can you simply grab the tx_entry from the back of the queue for now? I'm really against modifying txq_submit right now.
Quoted 14 lines
Quoted 14 lines
src/sms.c static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, · line 614
Quoted 10 lines
src/sms.c +10
-0+/* +* 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);Please avoid mixing tabs and spaces for indentation. And in this case it is more readale to break up the function than the equals line. E.g.
Quoted 7 lines
Quoted 3 lines
if (sms_address_to_hex_string(&receiver,
receiver_str == FALSE) return __ofono_error_failed(msg);This is truly a matter of personal opinion; I for one cannot see how this is more readable than the other form :)
All coding styles are a matter of personal taste. For oFono we prefer the above mentioned style.
Regards,
-Denis
Re: [patch 07/20] SMS: implement SHA256-based message IDs [incomplete]
Quoted 21 lines · Denis Kenzior
Quoted 20 lines · Inaky Perez-Gonzalez
Quoted 12 lines · Denis Kenzior
Hi Inaky,
src/ofono.h enum ofono_sms_submit_flag { · line 191
Quoted 6 lines
src/ofono.h +2
-2-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);I disagree with this, you're modifying an ofono internal API function to return a structure which is opaque and cannot be manipulated. I'd rather see you returning a struct
sms_uuid...I need a handle to operate on. The D-Bus wrappers need something they can use to dispose of the structure (call
tx_queue_entry_destroy()on the error path) and to pass tosms_msg_cancel(). The structure is not manipulated (except for accessing the UUID and #PDUs, which should probably be coded out to a helper).
Quoted 8 lines
Quoted 5 lines
This could be solved by having at the entry point of those functions or wrappers of a lookup function that looks for the UUID in the list of messages, but that would be adding more code and wasting time for no benefit. I disagree on adding unnecessary steps.
I need to look at this in detail. Can you simply grab the
tx_entryfrom the back of the queue for now?
Yes, that could be done, but it is way too dirty. Assumes *a lot* of knowledge about internal implementation details. A no-no.
Quoted 1 line
I'm really against modifying
txq_submitright now.
In any case it has to be modified to add the state change callback.
It has only *one* user asides from the SMS code and the changes don't impact said user negatively at all, so it shouldn't be such a big deal.
PATCH 8/20 sms: document the org.ofono.SMSMessage D-Bus interface
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
--- doc/sms-api.txt | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 47 insertions(+), 2 deletions(-)
doc/sms-api.txt +47 -2
@@ -22,9 +22,27 @@ Methods dict GetProperties() Possible Errors: [service].Error.InvalidArguments [service].Error.DoesNotExist - void SendMessage(string to, string text) + string SendMessage(string destination_number, string text) - Send the message in text to the number in to. + Submit a message for delivery / + processing. oFono owns it from now on until + successful delivery, cancellation (by the + user) or cancellation (by oFono). User has to + keep a copy around as oFono only offers + persistence to satisfy its needs. + + Returns the name of the D-Bus object that + represents said SMS Message. + + Possible Errors: [service].Error.InvalidArguments + + array{string} PendingMessages() + + Returns a list of SMS Messages that have been + submitted for delivery and that are still + pending. + + FIXME: currently not implemented Signals PropertyChanged(string name, variant value)
@@ -64,3 +82,30 @@ Properties string ServiceCenterAddress "ps-preferred" - Use CS if PS is unavailable By default oFono uses "cs-preferred" setting. + + +SMS / Messaging interface +========================= + +Service org.ofono +Interface org.ofono.SMSMessage +Object path [variable prefix]/modemX/message_id + +Methods dict GetProperties() + + Returns a dictionary with the current + properties of a message. + + void Cancel() + + Cancels a pending message's delivery. + +Signals PropertyChanged(string name, variant value) + + This signal indicates a changed value of the given + property. + +Properties string TXState + + Current transmission state. +
Signature · 1 line
1.6.6.1
Re: [patch 08/20] sms: document the org.ofono.SMSMessage D-Bus interface
Hi Inaky,
Quoted 11 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
--- doc/sms-api.txt | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 47 insertions(+), 2 deletions(-)doc/sms-api.txt +0
-0
doc/sms-api.txt Methods dict GetProperties() · line 25
Quoted 5 lines
doc/sms-api.txt +1
-1@@ -22,9 +22,27 @@ Methods dict GetProperties() Possible Errors: [service].Error.InvalidArguments [service].Error.DoesNotExist - void SendMessage(string to, string text) + string SendMessage(string destination_number, string text)
Actually keep string to, I like that better. The returned type should be 'object' (the object path of the new message) not 'string'.
doc/sms-api.txt Methods dict GetProperties() · line 35
Quoted 11 lines
doc/sms-api.txt +9
-1- Send the message in text to the number in to. + Submit a message for delivery / + processing. oFono owns it from now on until + successful delivery, cancellation (by the + user) or cancellation (by oFono). User has to + keep a copy around as oFono only offers + persistence to satisfy its needs. + + Returns the name of the D-Bus object that + represents said SMS Message.
Try to use tabs and not spaces for indentation.
doc/sms-api.txt Methods dict GetProperties() · line 39
Quoted 4 lines
doc/sms-api.txt +4
-0+ + Possible Errors: [service].Error.InvalidArguments + + array{string} PendingMessages()
array{object} here, and using Messages is better.
Quoted 10 lines
doc/sms-api.txt +6
-0+ + Returns a list of SMS Messages that have been + submitted for delivery and that are still + pending. + + FIXME: currently not implemented Signals PropertyChanged(string name, variant value)
doc/sms-api.txt Properties string ServiceCenterAddress · line 91
Quoted 10 lines
doc/sms-api.txt +7
-0@@ -64,3 +82,30 @@ Properties string ServiceCenterAddress "ps-preferred" - Use CS if PS is unavailable By default oFono uses "cs-preferred" setting. + + +SMS / Messaging interface +========================= + +Service org.ofono +Interface org.ofono.SMSMessage
oFono API is full CamelCase, so this should be SmsMessage.
doc/sms-api.txt Properties string ServiceCenterAddress · line 92
Quoted 1 line
doc/sms-api.txt +1
-0+Object path [variable prefix]/modemX/message_id
Please follow the conventions for Object Path, e.g. see doc/voicecall-api.txt
Quoted 9 lines
doc/sms-api.txt +9
-0+ +Methods dict GetProperties() + + Returns a dictionary with the current + properties of a message. + + void Cancel() + + Cancels a pending message's delivery.
doc/sms-api.txt Properties string ServiceCenterAddress · line 111
Quoted 10 lines
doc/sms-api.txt +10
-0+ +Signals PropertyChanged(string name, variant value) + + This signal indicates a changed value of the given + property. + +Properties string TXState + + Current transmission state. +
Rename this to State and include the possible state values please.
Regards,
-Denis
Re: [patch 08/20] sms: document the org.ofono.SMSMessage D-Bus interface
Quoted 17 lines · Denis Kenzior
Hi Inaky,
Quoted 14 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
--- doc/sms-api.txt | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 47 insertions(+), 2 deletions(-)doc/sms-api.txt +0
-0@@ -22,9 +22,27 @@ Methods dict GetProperties() Possible Errors: [service].Error.InvalidArguments [service].Error.DoesNotExist
Quoted 5 lines
doc/sms-api.txt Methods dict GetProperties() · line 25
Quoted 2 lines
doc/sms-api.txt +1
-1- void SendMessage(string to, string text) + string SendMessage(string destination_number, string text)Actually keep string to, I like that better. The returned type should be 'object' (the object path of the new message) not 'string'.
Changed
Quoted 13 lines
doc/sms-api.txt Methods dict GetProperties() · line 35
Quoted 11 lines
doc/sms-api.txt +9
-1- Send the message in text to the number in to. + Submit a message for delivery / + processing. oFono owns it from now on until + successful delivery, cancellation (by the + user) or cancellation (by oFono). User has to + keep a copy around as oFono only offers + persistence to satisfy its needs. + + Returns the name of the D-Bus object that + represents said SMS Message.Try to use tabs and not spaces for indentation.
Changed to tab (x8)
Quoted 6 lines
doc/sms-api.txt Methods dict GetProperties() · line 39
Quoted 4 lines
doc/sms-api.txt +4
-0+ + Possible Errors: [service].Error.InvalidArguments + + array{string} PendingMessages()array{object} here, and using Messages is better.
Done
Quoted 12 lines
Quoted 12 lines
doc/sms-api.txt +6
-0+ + Returns a list of SMS Messages that have been + submitted for delivery and that are still + pending. + + FIXME: currently not implemented Signals PropertyChanged(string name, variant value)@@ -64,3 +82,30 @@ Properties string ServiceCenterAddress "ps-preferred" - Use CS if PS is unavailable
Quoted 10 lines
doc/sms-api.txt Properties string ServiceCenterAddress · line 91
Quoted 8 lines
doc/sms-api.txt +7
-0By default oFono uses "cs-preferred" setting. + + +SMS / Messaging interface +========================= + +Service org.ofono +Interface org.ofono.SMSMessageoFono API is full CamelCase, so this should be SmsMessage.
I disagree. It would be like that if SMS was a word, but it is an acronym. It makes it quite confusing, if you ask me. If that's still how you like it, I'll change it, but I think it is wrong to capitalize an acronym.
Quoted 4 lines
doc/sms-api.txt Properties string ServiceCenterAddress · line 92
Quoted 1 line
doc/sms-api.txt +1
-0+Object path [variable prefix]/modemX/message_idPlease follow the conventions for Object Path, e.g. see
doc/voicecall-api.txt
Fixed
Quoted 13 lines
Quoted 13 lines
doc/sms-api.txt +13
-0+ +Methods dict GetProperties() + + Returns a dictionary with the current + properties of a message. + + void Cancel() + + Cancels a pending message's delivery. + +Signals PropertyChanged(string name, variant value) + + This signal indicates a changed value of the given
Quoted 8 lines
doc/sms-api.txt Properties string ServiceCenterAddress · line 111
Quoted 6 lines
doc/sms-api.txt +6
-0+ property. + +Properties string TXState + + Current transmission state. +Rename this to State and include the possible state values please.
Done
Re: [patch 08/20] sms: document the org.ofono.SMSMessage D-Bus interface
Hi Inaky,
Quoted 8 lines
Quoted 7 lines
doc/sms-api.txt Properties string ServiceCenterAddress · line 91
Quoted 5 lines
doc/sms-api.txt +5
-0+SMS / Messaging interface +========================= + +Service org.ofono +Interface org.ofono.SMSMessageoFono API is full CamelCase, so this should be SmsMessage.
Quoted 5 lines
I disagree. It would be like that if SMS was a word, but it is an acronym. It makes it quite confusing, if you ask me. If that's still how you like it, I'll change it, but I think it is wrong to capitalize an acronym.
Your objection has been noted. However, pure camel case looks nicer and that is how most of the API already is (e.g. SmsManager, SimManager, UnlockPin, etc) So please change it or name it simply 'Message'.
Regards,
-Denis
Re: [patch 08/20] sms: document the org.ofono.SMSMessage D-Bus interface
Quoted 16 lines · Denis Kenzior
Hi Inaky,
Quoted 8 lines
Quoted 7 lines
doc/sms-api.txt Properties string ServiceCenterAddress · line 91
Quoted 5 lines
doc/sms-api.txt +5
-0+SMS / Messaging interface +========================= + +Service org.ofono +Interface org.ofono.SMSMessageoFono API is full CamelCase, so this should be SmsMessage.
Quoted 5 lines
I disagree. It would be like that if SMS was a word, but it is an acronym. It makes it quite confusing, if you ask me. If that's still how you like it, I'll change it, but I think it is wrong to capitalize an acronym.
Quoted 3 lines
Your objection has been noted. However, pure camel case looks nicer and that is how most of the API already is (e.g. SmsManager, SimManager, UnlockPin, etc) So please change it or name it simply 'Message'.
Changed to SmsMessage as discussed over IRC.
PATCH 9/20 SMS: document handle_sms_status_report()
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
--- src/sms.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
src/sms.c +9 -0
@@ -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) {
Signature · 1 line
1.6.6.1
PATCH 10/20 sms_text_prepare: document @use_delivery_reports
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
--- src/smsutil.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
src/smsutil.c +3 -0
@@ -2831,6 +2831,9 @@ static inline GSList *sms_list_append(GSList *l, const struct sms *in)* Returns a list of sms messages in order. If ref_offset is given, * then the ref_offset contains the reference number offset or 0 * if no concatenation took place. +* +* @use_delivery_reports: value for the Status-Report-Request field +* (23.040 3.2.9, 9.2.2.2) */ GSList *sms_text_prepare(const char *utf8, guint16 ref, gboolean use_16bit, int *ref_offset,
Signature · 1 line
1.6.6.1
Re: [patch 10/20] sms_text_prepare: document @use_delivery_reports
Hi Inaky,
Quoted 6 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
--- src/smsutil.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
This one has been applied with a minor tweak to the commit message.
Regards,
-Denis
PATCH 11/20 SMS: rename create_tx_queue_entry() to tx_queue_entry_new()
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
This is for symmetry with tx_queue_entry_free()
--- src/sms.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
src/sms.c +3 -3
@@ -522,7 +522,7 @@ static void set_ref_and_to(GSList *msg_list, guint16 ref, int offset, } } -static struct tx_queue_entry *create_tx_queue_entry (GSList *msg_list) +static struct tx_queue_entry *tx_queue_entry_new (GSList *msg_list) { struct tx_queue_entry *entry = g_new0(struct tx_queue_entry, 1);int i =0 ;
@@ -574,7 +574,7 @@ static void send_message_destroy(void *data)* * 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 -* is created by create_tx_queue_entry() and g_queue_push_tail() +* 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. */
@@ -1338,7 +1338,7 @@ struct tx_queue_entry * __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *liofono_sms_txq_submit_cb_t cb, void *data, ofono_destroy_func destroy) { - struct tx_queue_entry *entry = create_tx_queue_entry(list); + struct tx_queue_entry *entry = tx_queue_entry_new(list);if (flags &OFONO_SMS_SUBMIT_FLAG_REQUEST_SR ) {struct sms *head = list->data;
Signature · 1 line
1.6.6.1
Re: [patch 11/20] SMS: rename create_tx_queue_entry() to tx_queue_entry_new()
Hi Inaky,
Quoted 7 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
This is for symmetry with
tx_queue_entry_free()--- src/sms.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Applied with a minor tweak to the commit header.
Regards,
-Denis
Re: [patch 11/20] SMS: rename create_tx_queue_entry() to tx_queue_entry_new()
Quoted 5 lines · Denis Kenzior
Hi Inaky,
Quoted 2 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
Quoted 7 lines
Quoted 5 lines
This is for symmetry with
tx_queue_entry_free()--- src/sms.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)Applied with a minor tweak to the commit header.
Please *don't* do that; remember the minor extra work it causes you multiplies by ten for the person that has to sort out the conflict in his git trees and figure out the kind of change you did.
PATCH 12/20 struct tx_queue_entry: add a destructor
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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(-)
src/sms.c +19 -6
@@ -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 ; }
Signature · 1 line
1.6.6.1
Re: [patch 12/20] struct tx_queue_entry: add a destructor
Hi Inaky,
src/sms.c static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg, · line 415
Quoted 8 lines
src/sms.c +8
-0+ +/* +* 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)
Can't this be simply tx_queue_entry_free? And feel free to take struct tx_queue_entry * here. Casting the destructor in g_foo_foreach is acceptable.
src/sms.c static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg, · line 420
Quoted 5 lines
src/sms.c +5
-0+{ +struct tx_queue_entry *entry = _entry; + +if (entry->destroy) + entry->destroy (entry->data);
An empty line after if/while/for blocks please. I will update the coding-style document with this rule shortly.
src/sms.c static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg, · line 424
Quoted 4 lines
src/sms.c +4
-0+g_free (entry->pdus); +g_free (entry); +} +
Regards,
-Denis
Re: [patch 12/20] struct tx_queue_entry: add a destructor
Quoted 12 lines · Denis Kenzior
Hi Inaky,
src/sms.c static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg, · line 415
Quoted 8 lines
src/sms.c +8
-0+ +/* +* 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)Can't this be simply
tx_queue_entry_free? And feel free to take struct
Sure, I'll rename that.
Quoted 2 lines
tx_queue_entry* here. Casting the destructor ing_foo_foreachis acceptable.
Heh, I'd rather have it like this: Repeat after me in a 1000-loop: casts are evil and should be avoided for they muck the code by hiding warnings when you change things and miss updating all the sites ...
Quoted 8 lines
src/sms.c static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg, · line 420
Quoted 5 lines
src/sms.c +5
-0+{ +struct tx_queue_entry *entry = _entry; + +if (entry->destroy) + entry->destroy (entry->data);An empty line after if/while/for blocks please. I will update the coding-style document with this rule shortly.
Will do, thanks
Re: [patch 12/20] struct tx_queue_entry: add a destructor
Hi Inaky,
Quoted 7 lines
Quoted 2 lines
tx_queue_entry* here. Casting the destructor ing_foo_foreachis acceptable.Heh, I'd rather have it like this: Repeat after me in a 1000-loop: casts are evil and should be avoided for they muck the code by hiding warnings when you change things and miss updating all the sites ...
I fully agree, but the extra silly NULL argument makes this worse than the cast. And to be clear, g_foo_foreach is about the only place we explicitly allow casts. If you feel you need to be pedantic, then write a GFunc specifically to do the foreach bit.
e.g. tx_queue_entry_free(struct tx_queue_entry *);
txq_foreach_free(gpointer foo, gpointer user)
{
struct tx_queue_entry *bar = foo;
tx_queue_entry_free(bar);
}
Regards,
-Denis
Re: [patch 12/20] struct tx_queue_entry: add a destructor
Quoted 15 lines · Denis Kenzior
Hi Inaky,
Quoted 7 lines
Quoted 2 lines
tx_queue_entry* here. Casting the destructor ing_foo_foreachis acceptable.Heh, I'd rather have it like this: Repeat after me in a 1000-loop: casts are evil and should be avoided for they muck the code by hiding warnings when you change things and miss updating all the sites ...
I fully agree, but the extra silly NULL argument makes this worse than the cast. And to be clear,
g_foo_foreachis about the only place we explicitly allow casts. If you feel you need to be pedantic, then write a GFunc specifically to do the foreach bit.
Quoted 8 lines
e.g.
tx_queue_entry_free(struct tx_queue_entry *);
txq_foreach_free(gpointer foo, gpointer user)
{
structtx_queue_entry*bar = foo;
tx_queue_entry_free(bar);
}
Done
Re: [patch 12/20] struct tx_queue_entry: add a destructor
In reply to Denis KenziorQuoted 14 lines · Denis Kenzior
Hi Inaky,
src/sms.c static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg, · line 415
Quoted 8 lines
src/sms.c +8
-0+ +/* +* 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)Can't this be simply
tx_queue_entry_free? And feel free to take structtx_queue_entry* here. Casting the destructor ing_foo_foreachis acceptable.
Renamed to tx_queue_entry_destroy() -- kept destroy vs free, as it has a clearer meaning as to what the function does.
Quoted 8 lines
src/sms.c static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg, · line 420
Quoted 5 lines
src/sms.c +5
-0+{ +struct tx_queue_entry *entry = _entry; + +if (entry->destroy) + entry->destroy (entry->data);An empty line after if/while/for blocks please. I will update the coding-style document with this rule shortly.
Done.
PATCH 13/20 SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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(-)
src/sms.c +19 -2
@@ -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)
Signature · 1 line
1.6.6.1
Re: [patch 13/20] SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'
Hi Inaky,
src/sms.c struct tx_queue_entry { · line 103
Quoted 11 lines
src/sms.c +11
-0+/* +* 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; +}; +
I don't really see a point for this structure. When you send an SMS, assuming the arguments and format were valid, you return straight away.
Thus SendMessage should be marked as not being ASYNC and storing of
DBusMessage is unnecessary. Rest can be handled by txq_submit and the dbus path can be easily generated based on the UUID.
src/sms.c static void send_message_destroy(void *data) · line 612
Quoted 11 lines
/*
src/sms.c +3
-0@@ -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)
This chunk seems totally unrelated...
Regards,
-Denis
Re: [patch 13/20] SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'
Quoted 11 lines · Denis Kenzior
Hi Inaky,
Quoted 9 lines
src/sms.c +9
-0+/* +* 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;
Quoted 8 lines
src/sms.c struct tx_queue_entry { · line 103
Quoted 2 lines
src/sms.c +2
-0+}; +I don't really see a point for this structure. When you send an SMS, assuming the arguments and format were valid, you return straight away.
Thus SendMessage should be marked as not being ASYNC and storing of
DBusMessage is unnecessary. Rest can be handled bytxq_submitand the dbus path can be easily generated based on the UUID.
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.
Quoted 5 lines
Quoted 5 lines
/*
src/sms.c +0
-0@@ -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.
Quoted 8 lines
src/sms.c static void send_message_destroy(void *data) · line 612
Quoted 6 lines
src/sms.c +3
-0+* +* @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)This chunk seems totally unrelated...
Yep, it is -- this should be a separate doc commit. Thanks for catching it.
Re: [patch 13/20] SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'
Hi Inaky,
Quoted 16 lines · Inaky Perez-Gonzalez
Quoted 14 lines · Denis Kenzior
Hi Inaky,
Quoted 5 lines
+/* + * 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 + */ +structsms_msg_dbus_data{ + char*dbus_path; + DBusMessage*dbus_msg;
+}; +I don't really see a point for this structure. When you send an SMS, assuming the arguments and format were valid, you return straight away. Thus SendMessage should be marked as not being ASYNC and storing of DBusMessage is unnecessary. Rest can be handled by
txq_submitand the dbus path can be easily generated based on the UUID.
Quoted 4 lines
First it serves as transitional storage -- later in the commit sequences the
dbus_msgis 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)
Don't get me wrong, if a structure is necessary in the end, so be it. However, during the review I only see a structure being introduced that is totally unnecessary. You can simply pass dbus_path around as userdata.
If/when this structure does become necessary, feel free to introduce it.
Quoted 8 lines
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
_PDUsrequirement:
There is no such requirement. If I gave you this idea, then I must have not been clear enough. I meant that PDUs would be used to generate the UUID.
Regards,
-Denis
Re: [patch 13/20] SMS: encapsulate D-Bus specific data in 'struct sms_msg_dbus_data'
Quoted 24 lines · Denis Kenzior
Hi Inaky,
Quoted 16 lines · Inaky Perez-Gonzalez
Quoted 14 lines · Denis Kenzior
Hi Inaky,
Quoted 5 lines
+/* + * 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 + */ +structsms_msg_dbus_data{ + char*dbus_path; + DBusMessage*dbus_msg;
+}; +I don't really see a point for this structure. When you send an SMS, assuming the arguments and format were valid, you return straight away. Thus SendMessage should be marked as not being ASYNC and storing of DBusMessage is unnecessary. Rest can be handled by
txq_submitand the dbus path can be easily generated based on the UUID.Quoted 4 lines
First it serves as transitional storage -- later in the commit sequences the
dbus_msgis 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)
Quoted 4 lines
Don't get me wrong, if a structure is necessary in the end, so be it. However, during the review I only see a structure being introduced that is totally unnecessary. You can simply pass
dbus_patharound as userdata.
Agreed -- at the end, once everything is cleaned up only dbus_path is necessary, and I agree it could be passed instead of having the struct. However, for all the intermediate commits not to break (and thus break bisectability), having that there is a must.
However, with so much reorganization in the code that has happened during this review process, it is probably possible to do with out it. I'll re-audit all that to see how it can be killed.
Quoted 10 lines
If/when this structure does become necessary, feel free to introduce it.
Quoted 7 lines
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.
Quoted 5 lines
Quoted 1 line
Even if we remove the
_PDUsrequirement:There is no such requirement. If I gave you this idea, then I must have not been clear enough. I meant that PDUs would be used to generate the UUID.
That simplifies things further, thanks.
PATCH 14/20 SMS: introduce bare state machine and transitions
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
This adds state to outgoing/in-transit SMS messages. This will be used later on for persistence / D-Bus, when the SMS life cycle is expanded.
The state is a variable in the 'struct tx_queue_entry' which gets updated as messages go through the hoops.
--- src/ofono.h | 33 ++++++++++++++++ src/sms.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 150 insertions(+), 4 deletions(-)
src/ofono.h +33 -0
@@ -183,6 +183,39 @@ enum ofono_sms_submit_flag { OFONO_SMS_SUBMIT_FLAG_RETRY = 0x4, }; +/* +* SMS TX message's state +* +* When a message is queued to be delivered, it will transition +* through a set of states. +* +* Allowed transition table (Allowed, Not-allowed) from left to right: +* +* UNINITIALIZED CANCELING FAILED +* | QUEUED WSR DONE | CANCELLED EXPIRED +* UNINITIALIZED - A N N N N N N +* QUEUED N - A A A N A N +* WSR N N - A A N A A +* DONE A N N - N N N N +* CANCELING N N N N - A A A +* CANCELLED A N N N N - N N +* FAILED A N N N N N - N +* EXPIRED A N N N N N N - +*/ +enum ofono_sms_tx_state { +OFONO_SMS_TX_ST_UNINITIALIZED , +OFONO_SMS_TX_ST_QUEUED , +OFONO_SMS_TX_ST_WSR , +OFONO_SMS_TX_ST_DONE , +OFONO_SMS_TX_ST_CANCELING , +OFONO_SMS_TX_ST_CANCELLED , +OFONO_SMS_TX_ST_FAILED , +OFONO_SMS_TX_ST_EXPIRED , + __OFONO_SMS_TX_ST_INVALID, +}; + +const char *ofono_sms_tx_state_to_string (enum ofono_sms_tx_state ); +typedef void (*ofono_sms_txq_submit_cb_t )(gboolean ok,void *data); struct tx_queue_entry * __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
src/sms.c +117 -4
190 lines not on this page · raw message
Signature · 1 line
1.6.6.1
PATCH 15/20 SMS: introduce Wait-for-Status-Report state and infrastructure
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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(-)
src/sms.c +61 -9
@@ -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);
Signature · 1 line
1.6.6.1
PATCH 16/20 SMS: introduce a state change callback for TX messages
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
When the SMS message going through the TX hoops changes states, call the given callback function (if any). This will be used later to hook up the D-Bus propagation of property changed signals.
Note this will takes away with the ofono_sms_txq_submit_cb callback, as it can be implemented in terms of a state change callback.
--- src/ofono.h | 5 +++-- src/sms.c | 36 +++++++++++++++++++++++++----------- src/stk.c | 22 ++++++++++++++++++---- 3 files changed, 46 insertions(+), 17 deletions(-)
src/ofono.h +3 -2
@@ -216,11 +216,12 @@ enum ofono_sms_tx_state {const char *ofono_sms_tx_state_to_string (enum ofono_sms_tx_state ); -typedef void (*ofono_sms_txq_submit_cb_t )(gboolean ok,void *data); +typedef void (*ofono_sms_msg_stch_cb_t )(void *data, +enum ofono_sms_tx_state new_state);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, +ofono_sms_msg_stch_cb_t stch_cb,void *data,ofono_destroy_func destroy);#include <ofono/sim.h>
src/sms.c +25 -11
@@ -114,7 +114,7 @@ struct tx_queue_entry {unsigned int msg_id;unsigned int retry;unsigned int flags; -ofono_sms_txq_submit_cb_t cb; +ofono_sms_msg_stch_cb_t stch_cb;void *data;ofono_destroy_func destroy; };
@@ -530,6 +530,8 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry,ofono_debug ("%s:%d: SMS state change: %p %u -> %u\n" , file, line, entry, state_old, state_new); entry->state = state_new; +if (entry->stch_cb) + entry->stch_cb (entry->data, state_new); }
@@ -602,10 +604,6 @@ 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); -if (entry->flags &OFONO_SMS_SUBMIT_FLAG_RECORD_HISTORY ) {enum ofono_history_sms_status hs;
@@ -700,12 +698,27 @@ static struct tx_queue_entry *tx_queue_entry_new(GSList *msg_list)return entry; } -static void send_message_cb (gboolean ok,void *data) +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);
@@ -795,9 +808,10 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,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_cb, -dbus_message_ref (msg), - send_message_destroy); + sms_msg =__ofono_sms_txq_submit (sms, msg_list, flags, msg_id, + send_message_stch_cb, +dbus_message_ref (msg), + send_message_destroy);g_slist_foreach (msg_list, (GFunc )g_free,NULL );g_slist_free (msg_list);
@@ -1528,7 +1542,7 @@ void *ofono_sms_get_data(struct ofono_sms *sms)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, +ofono_sms_msg_stch_cb_t stch_cb,void *data,ofono_destroy_func destroy) {struct tx_queue_entry *entry =tx_queue_entry_new (list);
@@ -1542,7 +1556,7 @@ struct tx_queue_entry * __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *li entry->msg_id = msg_id; entry->flags = flags; - entry->cb = cb; + entry->stch_cb = stch_cb; entry->data = data; entry->destroy = destroy; entry->sms_mgr = sms;
src/stk.c +17 -3
@@ -287,13 +287,27 @@ static void send_sms_cancel(struct ofono_stk *stk)stk_alpha_id_unset (stk); } -static void send_sms_submit_cb (gboolean ok,void *data) +static void send_sms_stch_cb (void *data,enum ofono_sms_tx_state new_state) {struct sms_submit_req *req = data;struct ofono_stk *stk = req->stk;struct ofono_error failure = { .type =OFONO_ERROR_TYPE_FAILURE };struct stk_response rsp; +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 ;ofono_debug ("SMS submission %s" , ok ?"successful" :"failed" );if (req->cancelled) {
@@ -339,8 +353,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 ;#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); +__ofono_sms_txq_submit (sms, &msg_list,0 ,0 , send_sms_stch_cb, + stk->sms_submit_req, g_free); stk->cancel_cmd = send_sms_cancel;
Signature · 1 line
1.6.6.1
PATCH 17/20 SMS: export outgoing messages over D-Bus
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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(-)
src/sms.c +133 -59
@@ -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 }, { } };
src/smsutil.h +12 -0
@@ -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"
Signature · 1 line
1.6.6.1
PATCH 18/20 SMS: send D-Bus SMS-MSG::PropertyChanged signals when message changes status
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
This hooks sms_msg_stch_dbus_cb() into the SMS state change callback so that a D-Bus signal will be emitted whenever an SMS Message transitions state. This allows a client to track, if desired, what is going on with the SMS Message it cares about.
--- src/sms.c | 36 +++++++++++++++++++++++++++++++++++- test/test-sms-msg-state-change | 24 ++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletions(-) create mode 100755 test/test-sms-msg-state-change
src/sms.c +35 -1
@@ -820,6 +820,40 @@ struct tx_queue_entry *sms_msg_send(/* +* Send a PropertyChange signal when the state changes +* +* D-Bus typing is "ss" for variable/value. +*/ +static +void sms_msg_stch_dbus_cb (void *_dbus_data, +enum ofono_sms_tx_state new_state) +{ +struct sms_msg_dbus_data *dbus_data = _dbus_data; +DBusConnection *dbus_conn =ofono_dbus_get_connection (); +DBusMessage *dbus_signal; +DBusMessageIter iter; +const char *property ="TXState" ; +const char *new_state_str; + +/* +* This will be called in the first transition, when all the +* D-Bus stuff is not yet in place. +*/ +if (dbus_data->dbus_path ==NULL ) +return ; + dbus_signal =dbus_message_new_signal ( + dbus_data->dbus_path,SMS_MSG_INTERFACE ,"PropertyChanged" ); +if (!dbus_signal) +return ; +dbus_message_iter_init_append (dbus_signal, &iter); +dbus_message_iter_append_basic (&iter,DBUS_TYPE_STRING , &property); + new_state_str =ofono_sms_tx_state_to_string (new_state); +dbus_message_iter_append_basic (&iter,DBUS_TYPE_STRING , &new_state_str); +g_dbus_send_message (dbus_conn, dbus_signal); +} + + +/* * D-Bus: Send a SMS text message * * @conn: D-Bus connection
@@ -846,7 +880,7 @@ static DBusMessage *dbus_sms_msg_send(DBusConnection *conn, DBusMessage *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_stch_dbus_cb, sms_msg_dbus_destroy, dbus_data);if (sms_msg ==NULL )
test/test-sms-msg-state-change new file +24 -0
@@ -0,0 +1,24 @@ +#!/usr/bin/python + +import gobject + +import dbus +import dbus.mainloop.glib + +def property_changed(name, value, path, interface): + if interface == "org.ofono.SMSMessage": + print "%s: name %s value %s interface %s" \ + % (path, name, value, interface) + +if __name__ == '__main__': + print "FIXME: this should test the whole state change transition machine" + dbus.mainloop.glib.DBusGMainLoop(set_as_default=True) + + bus = dbus.SystemBus() + + bus.add_signal_receiver( + property_changed, bus_name="org.ofono", + signal_name = "PropertyChanged", path_keyword="path", + interface_keyword="interface") + mainloop = gobject.MainLoop() + mainloop.run()
Signature · 1 line
1.6.6.1
PATCH 19/20 SMS: introduce sms_msg_cancel and its D-Bus wrapper
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
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
src/sms.c +63 -0
@@ -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 *
src/smsutil.h +15 -0
@@ -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. */
test/test-sms-msg-cancel new file +173 -0
173 lines not on this page · raw message
Signature · 1 line
1.6.6.1
Re: [patch 19/20] SMS: introduce sms_msg_cancel and its D-Bus wrapper
Hi Inaky,
Quoted 8 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
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.
Quoted 6 lines
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 ++++++++++++++++++++++++++++++++++++++++++++++
Multiple patches please. One for each directory.
Quoted 18 lines
3 files changed, 251 insertions(+), 0 deletions(-) create mode 100755 test/test-sms-msg-cancelsrc/sms.c +7
-0@@ -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;
src/sms.c static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, · line 557
Quoted 10 lines
src/sms.c +10
-0+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); +} + +
You're ignoring about every part of the coding standard in this chunk...
Quoted 28 lines
src/sms.c +28
-0+/** +* 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 )
src/sms.c struct tx_queue_entry *sms_msg_send( · line 884
Quoted 10 lines
src/sms.c +10
-0+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 ); +} + +
And again ignoring the coding standard...
Regards,
-Denis
Re: [patch 19/20] SMS: introduce sms_msg_cancel and its D-Bus wrapper
Quoted 22 lines · Denis Kenzior
Hi Inaky,
Quoted 8 lines · Inaky Perez-Gonzalez
From: Inaky Perez-Gonzalez <[email protected]>
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.
Quoted 6 lines
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 ++++++++++++++++++++++++++++++++++++++++++++++Multiple patches please. One for each directory.
...
Quoted 5 lines
You're ignoring about every part of the coding standard in this chunk...
...
And again ignoring the coding standard...
All feedback for this fixed, thanks.
PATCH 20/20 SMS: Implement D-Bus SMS-MSG::GetProperties
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
Currently this only returns the state of the SMS message.
--- src/sms.c | 25 +++++++++++++++++++++++++ 1 files changed, 25 insertions(+), 0 deletions(-)
src/sms.c +25 -0
@@ -537,6 +537,29 @@ static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, } +static DBusMessage *dbus_sms_msg_get_properties ( +DBusConnection * conn,DBusMessage *dbus_msg,void *_sms_msg) +{ +struct tx_queue_entry *sms_msg = _sms_msg; +DBusMessage *reply; +DBusMessageIter iter; +DBusMessageIter dict; +const char *str; + + reply =dbus_message_new_method_return (dbus_msg); +if (!reply) +return NULL ; +dbus_message_iter_init_append (reply, &iter); +dbus_message_iter_open_container (&iter,DBUS_TYPE_ARRAY , +OFONO_PROPERTIES_ARRAY_SIGNATURE , + &dict); + str =ofono_sms_tx_state_to_string (sms_msg->state); +ofono_dbus_dict_append (&dict,"TXState" ,DBUS_TYPE_STRING , &str); +dbus_message_iter_close_container (&iter, &dict); +return reply; +} + +/* * Note that the D-Bus specific cleanups are taken care by the * sms_msg_dbus_destroy() callback passed in dbus_sms_msg_send().
@@ -564,6 +587,8 @@ static DBusMessage *dbus_sms_msg_cancel(static GDBusMethodTable sms_msg_methods[] = { + {"GetProperties" ,"" ,"a{sv}" , + dbus_sms_msg_get_properties,G_DBUS_METHOD_FLAG_ASYNC }, {"Cancel" ,DBUS_TYPE_BOOLEAN_AS_STRING ,"" , dbus_sms_msg_cancel,0 }, { }
Signature · 1 line
1.6.6.1
Re: [patch 20/20] SMS: Implement D-Bus SMS-MSG::GetProperties
Hi Inaky,
src/sms.c static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, · line 551
Quoted 12 lines
src/sms.c +12
-0+static DBusMessage *dbus_sms_msg_get_properties ( +DBusConnection * conn,DBusMessage *dbus_msg,void *_sms_msg) +{ +struct tx_queue_entry *sms_msg = _sms_msg; +DBusMessage *reply; +DBusMessageIter iter; +DBusMessageIter dict; +const char *str; + + reply =dbus_message_new_method_return (dbus_msg); +if (!reply) +return NULL ;
Empty line here
src/sms.c static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, · line 555
Quoted 4 lines
src/sms.c +4
-0+dbus_message_iter_init_append (reply, &iter); +dbus_message_iter_open_container (&iter,DBUS_TYPE_ARRAY , +OFONO_PROPERTIES_ARRAY_SIGNATURE , + &dict);
Empty line here
src/sms.c static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, · line 557
Quoted 2 lines
src/sms.c +2
-0+ str =ofono_sms_tx_state_to_string (sms_msg->state); +ofono_dbus_dict_append (&dict,"TXState" ,DBUS_TYPE_STRING , &str);
Empty line here
src/sms.c static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, · line 558
Quoted 1 line
src/sms.c +1
-0+dbus_message_iter_close_container (&iter, &dict);
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?
src/sms.c static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, · line 562
Quoted 4 lines
src/sms.c +4
-0+return reply; +} + +
No double empty lines allowed.
src/sms.c static DBusMessage *dbus_sms_msg_cancel( · line 591
Quoted 9 lines
/*
* Note that the D-Bus specific cleanups are taken care by the
*sms_msg_dbus_destroy()callback passed indbus_sms_msg_send().src/sms.c +2
-0@@ -564,6 +587,8 @@ static DBusMessage *dbus_sms_msg_cancel(static GDBusMethodTable sms_msg_methods[] = { + {"GetProperties" ,"" ,"a{sv}" , + dbus_sms_msg_get_properties,G_DBUS_METHOD_FLAG_ASYNC },
Why exactly is this method ASYNC?
src/sms.c static DBusMessage *dbus_sms_msg_cancel( · line 594
Quoted 3 lines
src/sms.c +0
-0{"Cancel" ,DBUS_TYPE_BOOLEAN_AS_STRING ,"" , dbus_sms_msg_cancel, 0 }, { }
Regards,
-Denis
Re: [patch 20/20] SMS: Implement D-Bus SMS-MSG::GetProperties
Quoted 16 lines · Denis Kenzior
Hi Inaky,
src/sms.c static void __ofono_sms_tx_state_set(struct tx_queue_entry *entry, · line 551
Quoted 12 lines
src/sms.c +12
-0+static DBusMessage *dbus_sms_msg_get_properties ( +DBusConnection * conn,DBusMessage *dbus_msg,void *_sms_msg) +{ +struct tx_queue_entry *sms_msg = _sms_msg; +DBusMessage *reply; +DBusMessageIter iter; +DBusMessageIter dict; +const char *str; + + reply =dbus_message_new_method_return (dbus_msg); +if (!reply) +return NULL ;Empty line here
...
Quoted 1 line
Empty line here
...
Quoted 1 line
Empty line here
...
Quoted 1 line
And one more here
Done
Quoted 3 lines
Also, you might want to think about what other properties would be useful here. E.g. perhaps queue time, submission time, To address, contents?
None are needed so far
Contents will be quite messy, as there is only binary encoded data at this point and we might not even know how to decode it (let's say a binary list of PDUs vs a text message).
Other immediate ones are submission and delivery time, which also means we'd have to have a (property changed) signal for them and that'd require adding another callback in the core code [I am talking _txq_submit()]. I am going to leave it out for now because I don't want to add more complexity -- it's easy to add them later, but will require more changes.
Quoted 1 line
No double empty lines allowed.
fixed
Quoted 11 lines
src/sms.c static DBusMessage *dbus_sms_msg_cancel( · line 591
Quoted 9 lines
/*
* Note that the D-Bus specific cleanups are taken care by the
*sms_msg_dbus_destroy()callback passed indbus_sms_msg_send().src/sms.c +2
-0@@ -564,6 +587,8 @@ static DBusMessage *dbus_sms_msg_cancel(static GDBusMethodTable sms_msg_methods[] = { + {"GetProperties" ,"" ,"a{sv}" , + dbus_sms_msg_get_properties,G_DBUS_METHOD_FLAG_ASYNC },Why exactly is this method ASYNC?
Hmm, that was cut & paste from the manager's get_properties and it is obviously wrong -- fixed thanks