Re: [PATCH 04/11] Changes for SMS statur report.

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7273 bytes --]

Hi Pasi,

> From: Pasi Miettinen <pasi.miettinen@ixonos.com>
> 
> ---
>  include/sms.h |    2 +-
>  src/sms.c     |  124
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files 
changed,
>  119 insertions(+), 7 deletions(-)
> 
> diff --git a/include/sms.h b/include/sms.h
> index daaec37..c007675 100644
> --- a/include/sms.h
> +++ b/include/sms.h
> @@ -54,7 +54,7 @@ struct ofono_sms_driver {
> 
>  void ofono_sms_deliver_notify(struct ofono_sms *sms, unsigned char *pdu,
>  				int len, int tpdu_len);
> -void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
> +void ofono_sms_status_report_notify(struct ofono_sms *sms, unsigned char
>  *pdu, int len, int tpdu_len);

Please leave the rename out for now.

> 
>  int ofono_sms_driver_register(const struct ofono_sms_driver *d);
> diff --git a/src/sms.c b/src/sms.c
> index 855bef8..63e0190 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -459,9 +459,10 @@ static GDBusMethodTable sms_manager_methods[] = {
>  };
> 
>  static GDBusSignalTable sms_manager_signals[] = {
> -	{ "PropertyChanged",	"sv"		},
> -	{ "IncomingMessage",	"sa{sv}"	},
> -	{ "ImmediateMessage",	"sa{sv}"	},
> +	{ "PropertyChanged",		"sv"		},
> +	{ "IncomingMessage",		"sa{sv}"	},
> +	{ "ImmediateMessage",		"sa{sv}"	},
> +	{ "IncomingStatusReport",	"{sv}"		},

Your patch does too much here.  If you want to reformat this table, send a 
separate patch please.  The current patch should only add the new entry.

>  	{ }
>  };
> 
> @@ -471,6 +472,7 @@ static void dispatch_app_datagram(struct ofono_sms
>  *sms, int dst, int src, DBG("Got app datagram for dst port: %d, src port:
>  %d",
>  			dst, src);
>  	DBG("Contents-Len: %ld", len);
> +	//DBG("buf: %s", buf);

Submissions should never contain commented-out code, you either need it or you 
don't.

>  }
> 
>  static void dispatch_text_message(struct ofono_sms *sms,
> @@ -539,6 +541,74 @@ static void dispatch_text_message(struct ofono_sms
>  *sms, }
>  }
> 
> +static void dispatch_sms_delivery_report(struct ofono_sms *sms,
> +					const enum sms_st *st,
> +					const struct sms_address *raddr,
> +					const struct sms_scts *scts)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(sms->atom);
> +	DBusMessage *signal;
> +	DBusMessageIter iter;
> +	DBusMessageIter dict;
> +	char buf[128];
> +	const char *signal_name;
> +	time_t ts;
> +	struct tm remote;
> +	struct tm local;
> +	const char *str = buf;
> +
> +	if (!st){
> +		DBG("status unavailable");
> +		return;
> +	}
> +
> +	signal_name = "IncomingStatusReport";
> +
> +	signal = dbus_message_new_signal(path, OFONO_SMS_MANAGER_INTERFACE,
> +						signal_name);

This variable and assignment is not needed, simply use the string in the 
dbus_message_new_signal call.

> +
> +	if (!signal)
> +		return;
> +
> +

Two consecutive empty lines is a no-no ;)

> +	/*Start assembling dbus-message*/
> +	dbus_message_iter_init_append(signal, &iter);
> +
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +						&dict);
> +

Your signature here does not match the IncomingStatusReport signature.

> +	/*This is the time when sender sent the message,
> +	  should be delivery time?*/
> +	ts = sms_scts_to_time(scts, &remote);
> +	localtime_r(&ts, &local);
> +
> +	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &local);
> +	buf[127] = '\0';
> +	ofono_dbus_dict_append(&dict, "LocalSentTime", DBUS_TYPE_STRING, &str);
> +
> +	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &remote);
> +	buf[127] = '\0';
> +	ofono_dbus_dict_append(&dict, "SentTime", DBUS_TYPE_STRING, &str);

SentTime and LocalSentTime?

> +
> +	/*Status*/
> +	if(*st==0x00){
> +		str = sms_address_to_string(raddr);
> +		ofono_dbus_dict_append(&dict, "Message was delivered to",
>  DBUS_TYPE_STRING, &str); +	}
> +	else{
> +		str = sms_address_to_string(raddr);
> +		ofono_dbus_dict_append(&dict, "Message was not delivered to",
>  DBUS_TYPE_STRING, &str); +	}

I suggest using simple string types, like 'delivered', 'undeliverable', etc.  
Also, the status report enum has nice definitions on whether this is a final / 
non-final result, etc.  These should be handled appropriately.

> +
> +	/*dbus-message assembled*/
> +	dbus_message_iter_close_container(&iter, &dict);
> +
> +	g_dbus_send_message(conn, signal);
> +
> +}
> +
>  static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
>  {
>  	GSList *l;
> @@ -646,6 +716,18 @@ static void sms_dispatch(struct ofono_sms *sms, GSList
>  *sms_list) }
>  }
> 
> +static void sms_status_report_dispatch(struct ofono_sms *sms, GSList
>  *sms_list) +{
> +	const struct sms *s;
> +	enum sms_charset uninitialized_var(old_charset);

What is the purpose of this variable?

> +
> +	s = sms_list->data;
> +	dispatch_sms_delivery_report(sms, &s->status_report.st,
> +					&s->status_report.raddr,
> +					&s->status_report.scts);
> +
> +}
> +

Not quite sure why all of this is in a separate function...

>  static void handle_deliver(struct ofono_sms *sms, const struct sms
>  *incoming) {
>  	GSList *l;
> @@ -679,6 +761,19 @@ static void handle_deliver(struct ofono_sms *sms,
>  const struct sms *incoming) g_slist_free(l);
>  }
> 
> +static void handle_sms_status_report(struct ofono_sms *sms, const struct
>  sms *incoming) +{
> +	GSList *l;
> +
> +	/*TODO:
> +	fragmented SMS delivery report? check handle_deliver()
> +	*/

I suggest that we figure this part out first actually :)

> +
> +	l = g_slist_append(NULL, (void *)incoming);

Casting is not required.

> +	sms_status_report_dispatch(sms, l);
> +	g_slist_free(l);
> +}
> +
>  static inline gboolean handle_mwi(struct ofono_sms *sms, struct sms *s)
>  {
>  	gboolean discard;
> @@ -696,7 +791,7 @@ void ofono_sms_deliver_notify(struct ofono_sms *sms,
>  unsigned char *pdu, {
>  	struct sms s;
>  	enum sms_class cls;
> -
> +	DBG("ofono_sms_deliver_notify");

You should use a newline to separate the variable definitions and code.  If 
statement blocks should generally be preceded and followed by an empty line.

>  	if (!sms_decode(pdu, len, FALSE, tpdu_len, &s)) {
>  		ofono_error("Unable to decode PDU");
>  		return;
> @@ -807,10 +902,27 @@ out:
>  	handle_deliver(sms, &s);
>  }
> 
> -void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
> +void ofono_sms_status_report_notify(struct ofono_sms *sms, unsigned char
>  *pdu, int len, int tpdu_len)
>  {
> -	ofono_error("SMS Status-Report not yet handled");
> +	struct sms s;
> +	enum sms_class cls;
> +
> +	if (!sms_decode(pdu, len, FALSE, tpdu_len, &s)) {
> +		ofono_error("Unable to decode PDU");
> +		return;
> +	}
> +
> +	if (s.type != SMS_TYPE_STATUS_REPORT) {
> +		ofono_error("Expecting a STATUS REPORT pdu");
> +	}
> +
> +	if (!sms_dcs_decode(s.deliver.dcs, &cls, NULL, NULL, NULL)) {
> +		ofono_error("Unknown / Reserved DCS.  Ignoring");
> +		return;
> +	}
> +
> +	handle_sms_status_report(sms, &s);
>  }
> 
>  int ofono_sms_driver_register(const struct ofono_sms_driver *d)
> 

Regards,
-Denis