misc cleanups and SMS ID API
cover misc cleanups and SMS ID API
From: Inaky Perez-Gonzalez <[email protected]>
Hi All
This set of patches introduce:
- a few cleanups, code documentations nippets, .gitignore additions, header file dependency fixes in smutil.h
- adds BUILD_BUG_ON() [linux kernel style], printf-like attribute checking for {write,read}_file()
- make write_file() transaction-safe
- Introduce the SMS ID API (rfc, still incomplete).
feedback?
Thanks!
The following changes since commit 6b0f2328c5e190053f6a28ed119d01581e2119b3:
Denis Kenzior (1):
Update TODO
are available in the git repository at:
git://gitorious.org/~inakypg/ofono/ofono-inakypg.git master
Patches follow for reviewing convenience.
Inaky Perez-Gonzalez (13):
Update .gitignore to ignore cscope databases and backup files
sms_send_message: add a short roadmap
documentation: add note about referencing standards sms_assembly_add_fragment_backup: clarify how insertion spot is found
util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
smutil.h: add missing header file dependencies
write_file: make transaction-safe
storage: add __attribute__((format)) to {write,read}_file() for printf-like variable arg verification Add function doc headers to ofono_sms_{create,register}
doc: explain debugging options to -d, add a pointer in -h to manpage
automake: fix installation of udev rules in VPATH builds
SMS: introduce message ID API
SMS: implement SHA256-based message IDs [incomplete]
.gitignore | 3 + HACKING | 10 +++ Makefile.am | 7 +- doc/ofonod.8 | 5 +- doc/standards.txt | 8 ++ src/main.c | 4 +- src/sms.c | 69 +++++++++++++--- src/smsutil.c | 197 ++++++++++++++++++++++++++++++++++++++++++++ src/smsutil.h | 92 +++++++++++++++++++++ src/storage.c | 42 +++++++--- src/storage.h | 6 +- src/util.h | 29 +++++++ unit/test-sms-msg-id.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++ 13 files changed, 657 insertions(+), 27 deletions(-) create mode 100644 doc/standards.txt create mode 100644 unit/test-sms-msg-id.c
RFC PATCH 1/13 Update .gitignore to ignore cscope databases and backup files
From: Inaky Perez-Gonzalez <[email protected]>
--- .gitignore | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
.gitignore +3 -0
@@ -1,3 +1,4 @@ +*~ *.o *.lo *.la
@@ -16,6 +17,8 @@ config.sub configure depcomp compile +cscope.files +cscope.out install-sh libtool ltmain.sh
Signature · 1 line
1.6.6.1
Re: [RFC patches 01/13] Update .gitignore to ignore cscope databases and backup files
Hi Inaky,
.gitignore line 1
Quoted 6 lines
.gitignore +1
-0@@ -1,3 +1,4 @@ +*~
believe it or not, I always left that out on purpose. I like having git status show me all the stupid backup files my editor creates.
.gitignore config.sub · line 24
Quoted 12 lines
.gitignore +2
-0*.o *.lo *.la@@ -16,6 +17,8 @@ config.sub configure depcomp compile +cscope.files +cscope.out install-sh libtool ltmain.sh
Please put the cscope stuff behind autom4te.cache since it is not part of autoconf/automake.
Regards
Marcel
Re: [RFC patches 01/13] Update .gitignore to ignore cscope databases and backup files
Quoted 12 lines · Marcel Holtmann
Hi Inaky,
.gitignore line 1
Quoted 6 lines
.gitignore +1
-0@@ -1,3 +1,4 @@ +*~believe it or not, I always left that out on purpose. I like having git status show me all the stupid backup files my editor creates.
Quoted 16 lines
Quoted 9 lines
.gitignore +0
-0*.o *.lo *.la@@ -16,6 +17,8 @@ config.subconfigure
depcomp
compile.gitignore +2
-0+cscope.files +cscope.outQuoted 3 lines
install-sh
libtool
ltmain.shPlease put the cscope stuff behind autom4te.cache since it is not part of autoconf/automake.
Does this makes sense since this files are not related to the project itself but to specific tools? I usually put those on my personal gitignore file.
Signature · 2 lines
João Paulo Rechi Vita http://jprvita.wordpress.com/
Re: [RFC patches 01/13] Update .gitignore to ignore cscope databases and backup files
Hi Joao,
Quoted 27 lines
Quoted 10 lines
.gitignore line 1
Quoted 6 lines
.gitignore +1
-0@@ -1,3 +1,4 @@ +*~believe it or not, I always left that out on purpose. I like having git status show me all the stupid backup files my editor creates.
Quoted 16 lines
.gitignore config.sub · line 24
Quoted 12 lines
.gitignore +2
-0*.o *.lo *.la@@ -16,6 +17,8 @@ config.sub configure depcomp compile +cscope.files +cscope.out install-sh libtool ltmain.shPlease put the cscope stuff behind autom4te.cache since it is not part of autoconf/automake.
Quoted 3 lines
Does this makes sense since this files are not related to the project itself but to specific tools? I usually put those on my personal gitignore file.
actually I have to agree. They really don't belong in the project and the personal gitignore configuration is a suitable place for them.
Regards
Marcel
Re: [RFC patches 01/13] Update .gitignore to ignore cscope databases and backup files
Quoted 29 lines · Marcel Holtmann
Hi Joao,
Quoted 27 lines
Quoted 26 lines
.gitignore line 1
Quoted 6 lines
.gitignore +1
-0@@ -1,3 +1,4 @@ +*~believe it or not, I always left that out on purpose. I like having git status show me all the stupid backup files my editor creates.
.gitignore config.sub · line 24
Quoted 12 lines
.gitignore +2
-0*.o *.lo *.la@@ -16,6 +17,8 @@ config.sub configure depcomp compile +cscope.files +cscope.out install-sh libtool ltmain.shPlease put the cscope stuff behind autom4te.cache since it is not part of autoconf/automake.
Quoted 6 lines
Quoted 3 lines
Does this makes sense since this files are not related to the project itself but to specific tools? I usually put those on my personal gitignore file.
actually I have to agree. They really don't belong in the project and the personal gitignore configuration is a suitable place for them.
ACK, I'll drop this one then
RFC PATCH 2/13 sms_send_message: add a short roadmap
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
--- src/sms.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
src/sms.c +13 -0
@@ -398,6 +398,19 @@ static struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list)return entry; } +/* +* Pre-process a SMS text message and deliver it [D-BUS' SendMessage()] +* +* @conn: D-BUS connection +* @msg: message data (telephone number and text) +* @data: SMS object to use for transmision +* +* 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() +* appends that entry to the SMS transmit queue. Then the tx_next() +* function is scheduled to run to process the queue. +*/ static DBusMessage *sms_send_message (DBusConnection *conn,DBusMessage *msg,void *data) {
Signature · 1 line
1.6.6.1
Re: [RFC patches 02/13] sms_send_message: add a short roadmap
Hi Inaky,
Quoted 20 lines
From: Inaky Perez-Gonzalez <[email protected]>
--- src/sms.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)src/sms.c +6
-0@@ -398,6 +398,19 @@ static struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list)return entry; } +/* +* Pre-process a SMS text message and deliver it [D-BUS' SendMessage()] +* +* @conn: D-BUS connection +* @msg: message data (telephone number and text) +* @data: SMS object to use for transmision
src/sms.c static struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list) · line 416
Quoted 10 lines
src/sms.c +7
-0+* +* 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() +* appends that entry to the SMS transmit queue. Then the tx_next() +* function is scheduled to run to process the queue. +*/ static DBusMessage *sms_send_message (DBusConnection *conn,DBusMessage *msg,void *data) {
small comments here. So it is D-Bus. That is the official naming in documentation we settled on some time ago. Half of the documentation might be still using it wrongly, but we might wanna set a good example here.
I don't know how picky gtk-doc is about not having the : for title and the empty line after the title. However this is nicely readable inside the source code, so I am fine with it.
Regards
Marcel
Re: [RFC patches 02/13] sms_send_message: add a short roadmap
Quoted 29 lines · Marcel Holtmann
Hi Inaky,
Quoted 27 lines
From: Inaky Perez-Gonzalez <[email protected]>
--- src/sms.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)src/sms.c +13
-0@@ -398,6 +398,19 @@ static struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list)return entry; } +/* +* Pre-process a SMS text message and deliver it [D-BUS' SendMessage()] +* +* @conn: D-BUS connection +* @msg: message data (telephone number and text) +* @data: SMS object to use for transmision +* +* 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() +* 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 struct tx_queue_entry *create_tx_queue_entry(GSList *msg_list) · line 416
Quoted 3 lines
src/sms.c +0
-0static DBusMessage *sms_send_message (DBusConnection *conn,DBusMessage *msg,void *data) {small comments here. So it is D-Bus. That is the official naming in documentation we settled on some time ago. Half of the documentation might be still using it wrongly, but we might wanna set a good example here.
Ack to that -- I'll do a S&R for all of those.
Quoted 3 lines
I don't know how picky gtk-doc is about not having the : for title and the empty line after the title. However this is nicely readable inside the source code, so I am fine with it.
Actually I am thinking gtk-doc is probably not the best idea to settle on...it's quite a pain to generate and quite limited. More on this later.
Re: [RFC patches 02/13] sms_send_message: add a short roadmap
In reply to Inaky Perez-GonzalezHi Inaky,
Quoted 6 lines
From: Inaky Perez-Gonzalez <[email protected]>
--- src/sms.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
Applied with s/D-BUS/D-Bus
Regards,
-Denis
Re: [RFC patches 02/13] sms_send_message: add a short roadmap
Quoted 10 lines · Denis Kenzior
Hi Inaky,
Quoted 6 lines
From: Inaky Perez-Gonzalez <[email protected]>
--- src/sms.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)Applied with s/D-BUS/D-Bus
Thank you for applying, but please *don't* do it like that.
Complain and ask me to fix it, I'll be happy to comply and resend, but by modifying the commit yourself you drive git crazy and create conflicts between my commits and yours.
Now I have to go for a time-consuming process of finding which ones are and working around it. You might have saved some of your time in doing that, but it is way less that the one I am going to use on resolving the mess on each commit *plus* I don't get to really learn / set on stone what your preferences are so these review round trips become less frequent in the long term. We both loose here.
RFC PATCH 3/13 documentation: add note about referencing standards
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
--- doc/standards.txt | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) create mode 100644 doc/standards.txt
doc/standards.txt new file +8 -0
@@ -0,0 +1,8 @@ +Referencing standards in the source +=================================== + +When referencing standard documents use raw numbers xx.xxx for 3GPP +documents or xxx.xxx for ETSI document (eg: 23.040). If needing to +point to an specific section/subsection, explicitly say "Section foo" + +3GPP specs can be found in http://3gpp.org/ftp/Specs.
Signature · 1 line
1.6.6.1
Re: [RFC patches 03/13] documentation: add note about referencing standards
Hi Inaky,
Quoted 13 lines
From: Inaky Perez-Gonzalez <[email protected]>
--- doc/standards.txt | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) create mode 100644 doc/standards.txtdoc/standards.txt new file +0
-0
doc/standards.txt line 8
Quoted 8 lines
doc/standards.txt new file +8
-0@@ -0,0 +1,8 @@ +Referencing standards in the source +=================================== + +When referencing standard documents use raw numbers xx.xxx for 3GPP +documents or xxx.xxx for ETSI document (eg: 23.040). If needing to +point to an specific section/subsection, explicitly say "Section foo" + +3GPP specs can be found in http://3gpp.org/ftp/Specs.
would this not better fit into HACKING document. I don't really have a strong opinion here. Just asking.
Regards
Marcel
Re: [RFC patches 03/13] documentation: add note about referencing standards
Quoted 18 lines · Marcel Holtmann
Hi Inaky,
Quoted 16 lines
From: Inaky Perez-Gonzalez <[email protected]>
--- doc/standards.txt | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) create mode 100644 doc/standards.txtdoc/standards.txt new file +3
-0@@ -0,0 +1,8 @@ +Referencing standards in the source +=================================== +
Quoted 8 lines
doc/standards.txt line 8
Quoted 5 lines
doc/standards.txt new file +5
-0+When referencing standard documents use raw numbers xx.xxx for 3GPP +documents or xxx.xxx for ETSI document (eg: 23.040). If needing to +point to an specific section/subsection, explicitly say "Section foo" + +3GPP specs can be found in http://3gpp.org/ftp/Specs.would this not better fit into HACKING document. I don't really have a strong opinion here. Just asking.
This was Denis' request to have it as separate. I think initially I had put it on HACKING or README? It fits me in any, as long as it is easy to find.
RFC PATCH 4/13 sms_assembly_add_fragment_backup: clarify how insertion spot is found
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
--- src/smsutil.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
src/smsutil.c +5 -0
@@ -2480,6 +2480,11 @@ static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly,if (node->bitmap[offset] & bit)return NULL ; +/* Iterate over the bitmap to find in which position +* should the fragment be inserted -- basically we +* walk each bit in the bitmap until the bit we care +* about (offset:bit) and count which are stored -- +* that gives us in which position we have to insert. */ position =0 ;for (i =0 ; i < offset; i++)for (j =0 ; j <32 ; j++)
Signature · 1 line
1.6.6.1
Re: [RFC patches 04/13] sms_assembly_add_fragment_backup: clarify how insertion spot is found
Hi Inaky,
src/smsutil.c static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly, · line 2487
Quoted 5 lines
src/smsutil.c +5
-0+/* Iterate over the bitmap to find in which position +* should the fragment be inserted -- basically we +* walk each bit in the bitmap until the bit we care +* about (offset:bit) and count which are stored -- +* that gives us in which position we have to insert. */
Patch has been applied. However please keep your commit header to 50 characters or less. Also, the preferred style for multi-line comments is:
/* <empty line>
* ... comment
*/
Regards,
-Denis
Re: [RFC patches 04/13] sms_assembly_add_fragment_backup: clarify how insertion spot is found
Quoted 13 lines · Denis Kenzior
Hi Inaky,
src/smsutil.c static GSList *sms_assembly_add_fragment_backup(struct sms_assembly *assembly, · line 2487
Quoted 5 lines
src/smsutil.c +5
-0+/* Iterate over the bitmap to find in which position +* should the fragment be inserted -- basically we +* walk each bit in the bitmap until the bit we care +* about (offset:bit) and count which are stored -- +* that gives us in which position we have to insert. */ Patch has been applied. However please keep your commit header to 50 characters or less. Also, the preferred style for multi-line comments is:
/* <empty line>
* ... comment
*/
Ops, sorry -- I'll go fix those and provide updates for it.
RFC PATCH 5/13 util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
In reply to Inaky Perez-GonzalezFrom: 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/util.h | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-)
src/util.h +29 -0
@@ -77,3 +77,32 @@ unsigned char *pack_7bit(const unsigned char *in, long len, int byte_offset, long *items_written, unsigned char terminator);char *sim_string_to_utf8 (const unsigned char *buffer,int length); + +/* +* Build time consistency checks +* +* Stolen from the Linux kernel 2.6.35-rc; as most taken from the +* Linux kernel, this is licensed GPL v2 or newer. +*/ + +/* 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 stolen from the Linux kernel 2.6.35-rc. From now, code +* is GPL v2 only. +*/
Signature · 1 line
1.6.6.1
Re: [RFC patches 05/13] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
Hi Inaky,
Quoted 36 lines
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/util.h | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-)src/util.h +19
-0@@ -77,3 +77,32 @@ unsigned char *pack_7bit(const unsigned char *in, long len, int byte_offset, long *items_written, unsigned char terminator);char *sim_string_to_utf8 (const unsigned char *buffer,int length); + +/* +* Build time consistency checks +* +* Stolen from the Linux kernel 2.6.35-rc; as most taken from the +* Linux kernel, this is licensed GPL v2 or newer. +*/ + +/* 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
src/util.h unsigned char *pack_7bit(const unsigned char *in, long len, int byte_offset, · line 108
Quoted 10 lines
src/util.h +10
-0+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 stolen from the Linux kernel 2.6.35-rc. From now, code +* is GPL v2 only. +*/
sounds like a nice addition, but src/util.h is the wrong file. I prefer we make this compatible so that it can be applied to ConnMan and BlueZ as well.
I think something like include/bug.h is better.
Regards
Marcel
Re: [RFC patches 05/13] util.h: Add BUILD_BUG_ON() and friends for compile-time assert checking
Quoted 53 lines · Marcel Holtmann
Hi Inaky,
Quoted 36 lines
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/util.h | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-)src/util.h +19
-0@@ -77,3 +77,32 @@ unsigned char *pack_7bit(const unsigned char *in, long len, int byte_offset, long *items_written, unsigned char terminator);char *sim_string_to_utf8 (const unsigned char *buffer,int length); + +/* +* Build time consistency checks +* +* Stolen from the Linux kernel 2.6.35-rc; as most taken from the +* Linux kernel, this is licensed GPL v2 or newer. +*/ + +/* 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 src/util.h unsigned char *pack_7bit(const unsigned char *in, long len, int byte_offset, · line 108
Quoted 10 lines
src/util.h +10
-0+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 stolen from the Linux kernel 2.6.35-rc. From now, code +* is GPL v2 only. +*/ sounds like a nice addition, but
src/util.his the wrong file. I prefer we make this compatible so that it can be applied to ConnMan and BlueZ as well.
Quoted 1 line
I think something like
include/bug.his better.
Sounds ok to me. Denis?
RFC PATCH 6/13 smutil.h: add missing header file dependencies
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
--- src/smsutil.h | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
src/smsutil.h +10 -0
@@ -18,6 +18,14 @@* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA * */ +#ifndef __smsutil_h__ +#define __smsutil_h__ + +#include <time.h> +#include <glib/gtypes.h> +#include <glib/gslist.h> +#include <glib/gmessages.h> +#include <glib/gchecksum.h> #define CBS_MAX_GSM_CHARS 93
@@ -501,3 +509,5 @@ char *cbs_topic_ranges_to_string(GSList *ranges);GSList *cbs_extract_topic_ranges (const char *ranges);GSList *cbs_optimize_ranges (GSList *ranges);gboolean cbs_topic_in_range (unsigned int topic,GSList *ranges); + +#endif /* #ifndef __smsutil_h__ */
Signature · 1 line
1.6.6.1
Re: [RFC patches 06/13] smutil.h: add missing header file dependencies
Hi Inaky,
src/smsutil.h line 22
Quoted 10 lines
src/smsutil.h +2
-0@@ -18,6 +18,14 @@* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA * */ +#ifndef __smsutil_h__ +#define __smsutil_h__
please leave the circular dependency protection out here. I really want it to fail if we go so crazy and would require it.
Regards
Marcel
Re: [RFC patches 06/13] smutil.h: add missing header file dependencies
Quoted 15 lines · Marcel Holtmann
Hi Inaky,
src/smsutil.h line 22
Quoted 10 lines
src/smsutil.h +2
-0@@ -18,6 +18,14 @@* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA * */ +#ifndef __smsutil_h__ +#define __smsutil_h__please leave the circular dependency protection out here. I really want it to fail if we go so crazy and would require it.
I personally NAK here, but if you guys don't like it, I'll just drop this guy from the series. Not that critical.
Re: [RFC patches 06/13] smutil.h: add missing header file dependencies
In reply to Inaky Perez-GonzalezHi Inaky,
src/smsutil.h line 28
Quoted 4 lines
src/smsutil.h +4
-0+#include <glib/gtypes.h> +#include <glib/gslist.h> +#include <glib/gmessages.h> +#include <glib/gchecksum.h>
Isn't it sufficient to just add glib/glib.h here?
Regards,
-Denis
Re: [RFC patches 06/13] smutil.h: add missing header file dependencies
Quoted 8 lines · Denis Kenzior
Hi Inaky,
src/smsutil.h line 28
Quoted 4 lines
src/smsutil.h +4
-0+#include <glib/gtypes.h> +#include <glib/gslist.h> +#include <glib/gmessages.h> +#include <glib/gchecksum.h> Isn't it sufficient to just add
glib/glib.hhere?
You can definitely do that, but it also increases the compilation time...not that you or I would notice anyway.
RFC PATCH 7/13 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: [RFC patches 07/13] write_file: make transaction-safe
Hi Inaky,
src/storage.c ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, · line 143
Quoted 12 lines
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 +3
-0+/* 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. */
Again, prefer multiline comments to be in a certain format
src/storage.c ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, · line 155
Quoted 13 lines
src/storage.c +9
-0+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; }
And I get trouble applying your patch:
Applying: write_file: make transaction-safe /home/denkenz/ofono-master/.git/rebase-apply/patch:13: trailing whitespace. /*/home/denkenz/ofono-master/.git/rebase-apply/patch:22: trailing whitespace.
*//home/denkenz/ofono-master/.git/rebase-apply/patch:75: trailing whitespace.
Regards,
-Denis
Re: [RFC patches 07/13] write_file: make transaction-safe
Quoted 8 lines · Denis Kenzior
Hi Inaky,
Quoted 6 lines
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.
Quoted 8 lines
src/storage.c ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, · line 143
Quoted 6 lines
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 +3
-0+/* 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. */ Again, prefer multiline comments to be in a certain format
I'll fix this and resubmit.
Quoted 14 lines
src/storage.c ssize_t write_file(const unsigned char *buffer, size_t len, mode_t mode, · line 155
Quoted 13 lines
src/storage.c +9
-0+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; }
Quoted 7 lines
And I get trouble applying your patch:
Applying:write_file: make transaction-safe/home/denkenz/ofono-master/.git/rebase-apply/patch:13: trailing whitespace. /*/home/denkenz/ofono-master/.git/rebase-apply/patch:22: trailing whitespace.
*//home/denkenz/ofono-master/.git/rebase-apply/patch:75: trailing whitespace.
Ops, my bad. Ditto, I'll fix and resubmit.
RFC PATCH 8/13 storage: add __attribute__((format)) to {write, read}_file() for printf-like variable arg verification
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
--- src/storage.h | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
src/storage.h +4 -2
@@ -30,10 +30,12 @@int create_dirs (const char *filename,const mode_t mode);ssize_t read_file (unsigned char *buffer,size_t len, -const char *path_fmt, ...); +const char *path_fmt, ...) + __attribute__((format (printf,3 ,4 )));ssize_t write_file (const unsigned char *buffer,size_t len,mode_t mode, -const char *path_fmt, ...); +const char *path_fmt, ...) + __attribute__((format (printf,4 ,5 )));GKeyFile *storage_open (const char *imsi,const char *store);void storage_sync (const char *imsi,const char *store,GKeyFile *keyfile);
Signature · 1 line
1.6.6.1
Re: [RFC patches 08/13] storage: add __attribute__((format)) to {write, read}_file() for printf-like variable arg verification
Hi Inaky,
Quoted 6 lines
From: Inaky Perez-Gonzalez <[email protected]>
--- src/storage.h | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
Patch has been applied with a modified commit message.
Thanks,
-Denis
Re: [RFC patches 08/13] storage: add __attribute__((format)) to {write, read}_file() for printf-like variable arg verification
Quoted 10 lines · Denis Kenzior
Hi Inaky,
Quoted 6 lines
From: Inaky Perez-Gonzalez <[email protected]>
--- src/storage.h | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)Patch has been applied with a modified commit message.
Thanks.
Recording the same complain as before wrt to modifying commits contributed by others. Please mention which changes you want, let me do them and resubmit.
RFC PATCH 9/13 Add function doc headers to ofono_sms_{create, register}
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
--- src/sms.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-)
src/sms.c +19 -0
@@ -910,6 +910,17 @@ static void sms_remove(struct ofono_atom *atom)g_free (sms); } + +/* +* Create a SMS driver +* +* This creates a SMS driver that is hung off a @modem +* object. However, for the driver to be used by the system, it has to +* be registered with the oFono core using ofono_sms_register(). +* +* This is done once the modem driver determines that SMS is properly +* supported by the hardware. +*/ struct ofono_sms *ofono_sms_create(struct ofono_modem *modem, unsigned int vendor, const char *driver,
@@ -980,6 +991,14 @@ static void sms_load_settings(struct ofono_sms *sms, const char *imsi) } + +/* +* Indicate oFono that a SMS driver is ready for operation +* +* This is called after ofono_sms_create() was done and the modem +* driver determined that a modem supports SMS correctly. Once this +* call succeeds, the D-BUS interface for SMS goes live. +*/ void ofono_sms_register (struct ofono_sms *sms) {DBusConnection *conn =ofono_dbus_get_connection ();
Signature · 1 line
1.6.6.1
Re: [RFC patches 09/13] Add function doc headers to ofono_sms_{create, register}
Hi Inaky,
Quoted 3 lines
From: Inaky Perez-Gonzalez <[email protected]>
Signed-off-by Inaky Perez-Gonzalez <[email protected]>
We don't use signed off lines here, please don't include them in the future.
Patch has been applied with a modified commit message.
Thanks,
-Denis
Re: [RFC patches 09/13] Add function doc headers to ofono_sms_{create, register}
Quoted 7 lines · Denis Kenzior
Hi Inaky,
Quoted 3 lines
From: Inaky Perez-Gonzalez <[email protected]>
Signed-off-by Inaky Perez-Gonzalez <[email protected]>We don't use signed off lines here, please don't include them in the future.
Grr, sorry, my autopilot from the kernel keeps adding -s to 'git commit'. I added a tripcheck for it.
Quoted 1 line
Patch has been applied with a modified commit message.
Same complaint as before wrt to committing modified patches.
RFC PATCH 10/13 doc: explain debugging options to -d, add a pointer in -h to manpage
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 ++++- src/main.c | 4 +++- 3 files changed, 17 insertions(+), 2 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.
src/main.c +3 -1
@@ -98,7 +98,9 @@ static gboolean option_version = FALSE;static GOptionEntry options[] = { {"debug" ,'d' ,0 ,G_OPTION_ARG_STRING , &option_debug, -"Specify debug options to enable" ,"DEBUG" }, +"Specify debug options to enable (see the " +"man page for ofonod(8) for more information)." , +"DEBUG" }, {"nodetach" ,'n' ,G_OPTION_FLAG_REVERSE ,G_OPTION_ARG_NONE , &option_detach,"Don't run as daemon in background" },
Signature · 1 line
1.6.6.1
Re: [RFC patches 10/13] doc: explain debugging options to -d, add a pointer in -h to manpage
Hi Inaky,
Quoted 34 lines
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 ++++- src/main.c | 4 +++- 3 files changed, 17 insertions(+), 2 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 + debuggingdoc/ofonod.8 +0
-0@@ -18,7 +18,10 @@ is used to manage \fID-Bus\fP permissions for oFono. .SH OPTIONS
doc/ofonod.8 is used to manage \fID-Bus\fP permissions for oFono. · line 27
Quoted 10 lines
doc/ofonod.8 +4
-1.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.
you need to hook this up to automake :)
Quoted 5 lines
src/main.c +0
-0
src/main.c static gboolean option_version = FALSE; · line 106
Quoted 10 lines
src/main.c +3
-1@@ -98,7 +98,9 @@ static gboolean option_version = FALSE;static GOptionEntry options[] = { {"debug" ,'d' ,0 ,G_OPTION_ARG_STRING , &option_debug, -"Specify debug options to enable" ,"DEBUG" }, +"Specify debug options to enable (see the " +"man page for ofonod(8) for more information)." , +"DEBUG" }, {"nodetach" ,'n' ,G_OPTION_FLAG_REVERSE ,G_OPTION_ARG_NONE , &option_detach,"Don't run as daemon in background" },
Please leave this out. If we have a man page and man ofonod works, then this is not needed.
Regards
Marcel
Re: [RFC patches 10/13] doc: explain debugging options to -d, add a pointer in -h to manpage
Quoted 40 lines · Marcel Holtmann
Hi Inaky,
Quoted 38 lines
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 ++++- src/main.c | 4 +++- 3 files changed, 17 insertions(+), 2 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 + debuggingdoc/ofonod.8 +1
-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
Quoted 8 lines
doc/ofonod.8 is used to manage \fID-Bus\fP permissions for oFono. · line 27
Quoted 6 lines
doc/ofonod.8 +3
-0+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.you need to hook this up to automake :)
Can you clarify, please?
Quoted 6 lines
Quoted 6 lines
src/main.c +0
-0@@ -98,7 +98,9 @@ static gboolean option_version = FALSE;
Quoted 12 lines
src/main.c static gboolean option_version = FALSE; · line 106
Quoted 9 lines
src/main.c +3
-1static GOptionEntry options[] = { {"debug" ,'d' ,0 ,G_OPTION_ARG_STRING , &option_debug, -"Specify debug options to enable" ,"DEBUG" }, +"Specify debug options to enable (see the " +"man page for ofonod(8) for more information)." , +"DEBUG" }, {"nodetach" ,'n' ,G_OPTION_FLAG_REVERSE ,G_OPTION_ARG_NONE , &option_detach,"Don't run as daemon in background" },Please leave this out. If we have a man page and man ofonod works, then this is not needed.
Denis and me, AFAIR, agreed on this wording as it just gives the quick pointer to where the extra information is found. Again, not critical, but confusing feedback.
Re: [RFC patches 10/13] doc: explain debugging options to -d, add a pointer in -h to manpage
Hi Inaky,
Quoted 47 lines
Quoted 38 lines
Quoted 38 lines
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 ++++- src/main.c | 4 +++- 3 files changed, 17 insertions(+), 2 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 + debuggingdoc/ofonod.8 +1
-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 beQuoted 8 lines
doc/ofonod.8 is used to manage \fID-Bus\fP permissions for oFono. · line 27
Quoted 6 lines
doc/ofonod.8 +3
-0+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.you need to hook this up to automake :)
Quoted 1 line
Can you clarify, please?
we do wanna install the man pages, right? Then this needs man_MANS and EXTRA_DIST magic to get included and installed.
Quoted 19 lines
Quoted 6 lines
Quoted 6 lines
src/main.c +0
-0@@ -98,7 +98,9 @@ static gboolean option_version = FALSE;Quoted 12 lines
src/main.c static gboolean option_version = FALSE; · line 106
Quoted 9 lines
src/main.c +3
-1static GOptionEntry options[] = { {"debug" ,'d' ,0 ,G_OPTION_ARG_STRING , &option_debug, -"Specify debug options to enable" ,"DEBUG" }, +"Specify debug options to enable (see the " +"man page for ofonod(8) for more information)." , +"DEBUG" }, {"nodetach" ,'n' ,G_OPTION_FLAG_REVERSE ,G_OPTION_ARG_NONE , &option_detach,"Don't run as daemon in background" },Please leave this out. If we have a man page and man ofonod works, then this is not needed.
Quoted 3 lines
Denis and me, AFAIR, agreed on this wording as it just gives the quick pointer to where the extra information is found. Again, not critical, but confusing feedback.
Maybe I am too old school Linux here ;)
If we have an installed man page and I don't understand the usage information, then it is clear to just do man ofonod. I don't need the usage tell me that.
Regards
Marcel
Re: [RFC patches 10/13] doc: explain debugging options to -d, add a pointer in -h to manpage
Quoted 49 lines · Marcel Holtmann
Hi Inaky,
Quoted 47 lines
Quoted 46 lines
Quoted 34 lines
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 ++++- src/main.c | 4 +++- 3 files changed, 17 insertions(+), 2 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 + debuggingdoc/ofonod.8 +0
-0@@ -18,7 +18,10 @@ is used to manage \fID-Bus\fP permissions for oFono. .SH OPTIONSdoc/ofonod.8 is used to manage \fID-Bus\fP permissions for oFono. · line 27
Quoted 10 lines
doc/ofonod.8 +4
-1.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.you need to hook this up to automake :)
Quoted 4 lines
Quoted 1 line
Can you clarify, please?
we do wanna install the man pages, right? Then this needs
man_MANSandEXTRA_DISTmagic to get included and installed.
Hmm, I didn't realize the makefile magic wasn't plugged in--I just patched the man page.
Ack to that, will add that as soon as I have the tree rebased from the modified commits.
Quoted 23 lines
Quoted 19 lines
Quoted 6 lines
Quoted 6 lines
src/main.c +0
-0@@ -98,7 +98,9 @@ static gboolean option_version = FALSE;Quoted 12 lines
src/main.c static gboolean option_version = FALSE; · line 106
Quoted 9 lines
src/main.c +3
-1static GOptionEntry options[] = { {"debug" ,'d' ,0 ,G_OPTION_ARG_STRING , &option_debug, -"Specify debug options to enable" ,"DEBUG" }, +"Specify debug options to enable (see the " +"man page for ofonod(8) for more information)." , +"DEBUG" }, {"nodetach" ,'n' ,G_OPTION_FLAG_REVERSE ,G_OPTION_ARG_NONE , &option_detach,"Don't run as daemon in background" },Please leave this out. If we have a man page and man ofonod works, then this is not needed.
Quoted 3 lines
Denis and me, AFAIR, agreed on this wording as it just gives the quick pointer to where the extra information is found. Again, not critical, but confusing feedback.
Quoted 5 lines
Maybe I am too old school Linux here ;)
If we have an installed man page and I don't understand the usage information, then it is clear to just do man ofonod. I don't need the usage tell me that.
heh -- yeah, I tend to do that too. Anyhow, let's leave it up to Denis.
RFC PATCH 11/13 automake: fix installation of udev rules in VPATH builds
In reply to Inaky Perez-GonzalezFrom: Inaky Perez-Gonzalez <[email protected]>
When the build directory is different than the source directory, we need to specify the source prefix to the original file we are copying.
--- Makefile.am | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
Makefile.am +1 -1
@@ -418,7 +418,7 @@ src/ofono.ver: src/ofono.exp $(AM_V_at)echo "local: *; };" >> $@ plugins/%.rules: - $(AM_V_GEN)cp $(subst 97-,,$@) $@ + $(AM_V_GEN)cp $(srcdir)/$(subst 97-,,$@) $@ $(src_ofonod_OBJECTS) $(unit_objects): $(local_headers)
Signature · 1 line
1.6.6.1
Re: [RFC patches 11/13] automake: fix installation of udev rules in VPATH builds
Hi Inaky,
Quoted 12 lines
When the build directory is different than the source directory, we need to specify the source prefix to the original file we are copying.
--- Makefile.am | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)Makefile.am +0
-0
Makefile.am src/ofono.ver: src/ofono.exp · line 423
Quoted 7 lines
Makefile.am +1
-1@@ -418,7 +418,7 @@ src/ofono.ver: src/ofono.exp $(AM_V_at)echo "local: *; };" >> $@ plugins/%.rules: - $(AM_V_GEN)cp $(subst 97-,,$@) $@ + $(AM_V_GEN)cp $(srcdir)/$(subst 97-,,$@) $@ $(src_ofonod_OBJECTS) $(unit_objects): $(local_headers)
I tested it within my release build system and it looks fine to me. At least nothing breaks. I leave it up to Denis to apply the patch, but please also submit similar patches for BlueZ and ConnMan since I am pretty sure I missed it there as well.
Regards
Marcel
Re: [RFC patches 11/13] automake: fix installation of udev rules in VPATH builds
In reply to Inaky Perez-GonzalezHi Inaky,
Quoted 5 lines
From: Inaky Perez-Gonzalez <[email protected]>
When the build directory is different than the source directory, we need to specify the source prefix to the original file we are copying.
Patch has been applied with a modified commit message.
Thanks,
-Denis
RFC PATCH 12/13 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.
--- Makefile.am | 5 +- src/smsutil.c | 192 +++++++++++++++++++++++++++++++++++++++++++ src/smsutil.h | 82 +++++++++++++++++++ unit/test-sms-msg-id.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 490 insertions(+), 1 deletions(-) create mode 100644 unit/test-sms-msg-id.c
Makefile.am +4 -1
@@ -345,7 +345,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@
@@ -384,6 +384,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 +192 -0
195 lines not on this page · raw message
src/smsutil.h +82 -0
86 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: [RFC patches 12/13] SMS: introduce message ID API
Hi Inaky,
Quoted 11 lines
+/**
+ * Create a random UUID
+ *
+ * \parammsg_idDescriptor
+ *
+ * \internal
+ *
+ * Crams themsg_idID buffer with random 32-bit numbers; because we
+ * use a byte based buffer, we play the cast to guint32 trick (as that
+ * is whatg_random_int()returns).
+ */
can we just stick with gtk-doc like you had in your previous patches. I think intermixing it with doxygen is a bad idea.
Regards
Marcel
Re: [RFC patches 12/13] SMS: introduce message ID API
Quoted 14 lines · Marcel Holtmann
Hi Inaky,
Quoted 11 lines
+/**
+ * Create a random UUID
+ *
+ * \parammsg_idDescriptor
+ *
+ * \internal
+ *
+ * Crams themsg_idID buffer with random 32-bit numbers; because we
+ * use a byte based buffer, we play the cast to guint32 trick (as that
+ * is whatg_random_int()returns).
+ */
Quoted 2 lines
can we just stick with gtk-doc like you had in your previous patches. I think intermixing it with doxygen is a bad idea.
Yep, my bad, sorry.
In any case, picking this up from earlier:
I have my doubts on gtk-doc vs doxygen. I remember there was a thread somewhere (email? irc? voice?) on which system to settle on which never got finalized. I am not too satisfied with gtk-doc's limitations, I'd rather do doxygen. Does anyone have any preference on once vs the other?
In any case, when we settle on one, I'll have to go and cleanup all these to conform to it. I'll also write a quick summary with examples on how to use it in doc/.
Re: [RFC patches 12/13] SMS: introduce message ID API
Hi Inaky,
Quoted 19 lines
Quoted 12 lines
Quoted 11 lines
+/**
+ * Create a random UUID
+ *
+ * \parammsg_idDescriptor
+ *
+ * \internal
+ *
+ * Crams themsg_idID buffer with random 32-bit numbers; because we
+ * use a byte based buffer, we play the cast to guint32 trick (as that
+ * is whatg_random_int()returns).
+ */Quoted 2 lines
can we just stick with gtk-doc like you had in your previous patches. I think intermixing it with doxygen is a bad idea.
Yep, my bad, sorry.
In any case, picking this up from earlier:
Quoted 8 lines
I have my doubts on gtk-doc vs doxygen. I remember there was a thread somewhere (email? irc? voice?) on which system to settle on which never got finalized. I am not too satisfied with gtk-doc's limitations, I'd rather do doxygen. Does anyone have any preference on once vs the other?
In any case, when we settle on one, I'll have to go and cleanup all these to conform to it. I'll also write a quick summary with examples on how to use it in doc/.
my preference with gtk-doc is that it generates nicer documentation and also makes devhelp files available.
Regards
Marcel
RFC PATCH 13/13 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: not yet complete, need to figure out how to do so that identical messages sent to the same number also have a different ID.
--- src/sms.c | 37 +++++++++++++++++++++++++++---------- 1 files changed, 27 insertions(+), 10 deletions(-)
src/sms.c +27 -10
@@ -55,7 +55,6 @@ struct ofono_sms {DBusMessage *pending;struct ofono_phone_number sca;struct sms_assembly *assembly; -unsigned int next_msg_id;guint ref;GQueue *txq;time_t last_mms;
@@ -81,6 +80,7 @@ struct tx_queue_entry {unsigned char num_pdus;unsigned char cur_pdu;unsigned int msg_id; +struct sms_msg_id sms_msg_id;unsigned int retry;DBusMessage *msg; };
@@ -421,6 +421,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,int ref_offset;struct tx_queue_entry *entry;struct ofono_modem *modem; +DECLARE_SMS_MSG_ID_STRBUF (msg_id_str); if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to, DBUS_TYPE_STRING, &text,
@@ -451,7 +452,17 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg, } entry->msg =dbus_message_ref (msg); - entry->msg_id = sms->next_msg_id++; +sms_msg_id_init (&entry->sms_msg_id); +sms_msg_id_hash (&entry->sms_msg_id, text,strlen (text)); +sms_msg_id_hash (&entry->sms_msg_id, to,strlen (to)); +#warning FIXME: add source time or something? otherwise repeats are the same; using ref for now, doesn't really work +sms_msg_id_hash (&entry->sms_msg_id, &sms->ref,sizeof (sms->ref)); +sms_msg_id_hash (&entry->sms_msg_id,NULL ,0 ); +sms_msg_id_memcpy (&entry->msg_id,sizeof (entry->msg_id), + &entry->sms_msg_id); +sms_msg_id_printf (&entry->sms_msg_id, msg_id_str,sizeof (msg_id_str)); +ofono_debug ("SMS TX: ref %08x, msg id: %s\n" , + sms->ref, msg_id_str);g_queue_push_tail (sms->txq, entry);
@@ -508,6 +519,9 @@ static void dispatch_text_message(struct ofono_sms *sms,struct tm remote;struct tm local;const char *str = buf; +struct sms_msg_id sms_msg_id; +unsigned int id; +DECLARE_SMS_MSG_ID_STRBUF (msg_id_str);if (!message)return ;
@@ -549,11 +563,18 @@ static void dispatch_text_message(struct ofono_sms *sms,g_dbus_send_message (conn, signal); -if (cls !=SMS_CLASS_0 ) { -__ofono_history_sms_received (modem, sms->next_msg_id, str, +sms_msg_id_init (&sms_msg_id); +sms_msg_id_hash (&sms_msg_id, message,strlen (message)); +sms_msg_id_hash (&sms_msg_id, addr->address,strlen (addr->address)); +#warning FIXME: add RX time? +sms_msg_id_hash (&sms_msg_id,NULL ,0 ); +sms_msg_id_memcpy (&id,sizeof (id), &sms_msg_id); +sms_msg_id_printf (&sms_msg_id, msg_id_str,sizeof (msg_id_str)); +ofono_debug ("SMS RX: msg id is %s\n" , msg_id_str); + +if (cls !=SMS_CLASS_0 ) +__ofono_history_sms_received (modem, id, str, &remote, &local, message); - sms->next_msg_id +=1 ; - } }static void sms_dispatch (struct ofono_sms *sms,GSList *sms_list)
@@ -896,8 +917,6 @@ static void sms_remove(struct ofono_atom *atom)if (sms->settings) {g_key_file_set_integer (sms->settings,SETTINGS_GROUP , -"NextMessageId" , sms->next_msg_id); -g_key_file_set_integer (sms->settings,SETTINGS_GROUP ,"NextReference" , sms->ref);storage_close (sms->imsi,SETTINGS_STORE , sms->settings, TRUE);
@@ -981,8 +1000,6 @@ static void sms_load_settings(struct ofono_sms *sms, const char *imsi) sms->imsi =g_strdup (imsi); - sms->next_msg_id =g_key_file_get_integer (sms->settings,SETTINGS_GROUP , -"NextMessageId" ,NULL ); sms->ref =g_key_file_get_integer (sms->settings,SETTINGS_GROUP ,"NextReference" ,NULL );
Signature · 1 line
1.6.6.1