[PATCH 1/2] smsutil: sms_decode() allow to pass sms with unknown TPDU size

[PATCH 1/2] smsutil: sms_decode() allow to pass sms with unknown TPDU size

[Alexander Couzens]

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

Certain modems (QMI based) only return a PDU length, but without
the length of the TPDU.
Without passing this information over ofono_sms_deliver_notify() down
to sms_decode(), sms_decode() will fail, as it try to interpret the
SC header as PDU header.
---

I'm not quite sure this is the best way to implement this.

a) adding a flag to ofono_sms_deliver_notify() would break the api
b) export sms_decode_address_field() which is part of smsutil.h would mean
   using this function only to get the length of TPDU and throw away the
   result (address), which is later called a second time.
c) add another function to cover this case is also bloated


 include/sms.h | 2 ++
 src/smsutil.c | 8 +++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/sms.h b/include/sms.h
index e9260561..602042d2 100644
--- a/include/sms.h
+++ b/include/sms.h
@@ -60,6 +60,8 @@ struct ofono_sms_driver {
 				ofono_sms_bearer_set_cb_t, void *data);
 };
 
+/* unknown tpdu length, but contains a SC header */
+#define TPDU_UNKNOWN_WITH_SC -2
 void ofono_sms_deliver_notify(struct ofono_sms *sms, const unsigned char *pdu,
 				int len, int tpdu_len);
 void ofono_sms_status_notify(struct ofono_sms *sms, const unsigned char *pdu,
diff --git a/src/smsutil.c b/src/smsutil.c
index 24dcfaa7..d4b4a0d4 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -33,6 +33,7 @@
 #include <unistd.h>
 
 #include <glib.h>
+#include <ofono/sms.h>
 
 #include "util.h"
 #include "storage.h"
@@ -1545,7 +1546,12 @@ gboolean sms_decode(const unsigned char *pdu, int len, gboolean outgoing,
 
 	memset(out, 0, sizeof(*out));
 
-	if (tpdu_len < len) {
+	if (tpdu_len == TPDU_UNKNOWN_WITH_SC) {
+		/* unknown TPDU length but prefixed with SC */
+		sms_decode_address_field(pdu, len, &offset, TRUE,
+				&out->sc_addr);
+		tpdu_len = len - offset;
+	} else if (tpdu_len < len) {
 		if (!sms_decode_address_field(pdu, len, &offset,
 						TRUE, &out->sc_addr))
 			return FALSE;
-- 
2.13.0

[PATCH 2/2] qmimodem: receive & delete sms from NV storage on 'Powered'

[Alexander Couzens]

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

When the modem is powered download all message from the storage
and delete them afterwards. This is required as certain modems
(e.g. Gobi 2000) won't receive any further messages and generate
a SMPP RP-Error 'Protocol error, unspecific'. Even those messages
should not saved in the ME.

This should also the case for newer modems, like the EC20, but they
have a much bigger storage as it doesn't got noticed.
Storage capacity: EC20: 255 msg, Gobi2000: 23 msg.
---
 drivers/qmimodem/sms.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/qmimodem/wms.h |  55 ++++++++++++++--
 2 files changed, 219 insertions(+), 5 deletions(-)

diff --git a/drivers/qmimodem/sms.c b/drivers/qmimodem/sms.c
index 43bf22d1..4df65962 100644
--- a/drivers/qmimodem/sms.c
+++ b/drivers/qmimodem/sms.c
@@ -334,6 +334,170 @@ error:
 	g_free(cbd);
 }
 
+static void delete_message(struct ofono_sms *sms, uint32_t message_id)
+{
+	struct qmi_param *param = qmi_param_new();
+	struct sms_data *data = ofono_sms_get_data(sms);
+
+	if (!param)
+		return;
+
+	DBG("remove message %08x", message_id);
+
+	qmi_param_append_uint8(param, QMI_WMS_PARAM_DELETE_STORAGE,
+			       QMI_WMS_STORAGE_TYPE_NV);
+	qmi_param_append_uint32(param, QMI_WMS_PARAM_DELETE_MESSAGE_INDEX,
+			       message_id);
+	qmi_param_append_uint8(param, QMI_WMS_PARAM_DELETE_MESSAGE_MODE,
+			 QMI_WMS_MESSAGE_MODE_GSMWCDMA);
+
+	if (qmi_service_send(data->wms, QMI_WMS_DELETE_MESSAGE, param,
+					NULL, NULL, NULL) > 0)
+		return;
+}
+
+struct read_message_data {
+	struct ofono_sms *sms;
+	uint32_t message_id;
+};
+
+static void read_message_cb(struct qmi_result *result, void *user_data)
+{
+	struct read_message_data *read_data = user_data;
+	const struct qmi_wms_raw_message *message;
+	uint16_t len;
+	uint16_t plen;
+
+	message = qmi_result_get(result, QMI_WMS_PARAM_RAW_READ_MESSAGE, &len);
+	if (!message)
+		return;
+
+	/* -1 when the payload length is 0 */
+	if (sizeof(*message) - 1 > len)
+		return;
+
+	plen = GUINT16_FROM_LE(message->count);
+	if (plen + 4 > len)
+		return;
+
+	switch (message->message_tag) {
+	case QMI_WMS_MESSAGE_TAG_TYPE_MT_NOT_READ:
+	case QMI_WMS_MESSAGE_TAG_TYPE_MT_READ:
+		ofono_sms_deliver_notify(read_data->sms,
+						message->data,
+						plen,
+						TPDU_UNKNOWN_WITH_SC);
+		break;
+	default:
+		break;
+	}
+
+	delete_message(read_data->sms, read_data->message_id);
+	g_free(read_data);
+}
+
+static void read_delete_message(struct ofono_sms *sms, uint32_t message_id)
+{
+	struct sms_data *data = ofono_sms_get_data(sms);
+	struct qmi_wms_result_new_msg_notify read_message;
+	struct qmi_param *param = NULL;
+	struct read_message_data *read_data = NULL;
+
+	DBG("");
+
+	read_data = g_try_new0(struct read_message_data, 1);
+	if (!read_data)
+		return;
+	read_data->sms = sms;
+	read_data->message_id = message_id;
+
+	param = qmi_param_new();
+	if (!param)
+		goto err;
+
+	read_message.storage_index = GUINT32_TO_LE(message_id);
+	read_message.storage_type = QMI_WMS_STORAGE_TYPE_NV;
+
+	if (!qmi_param_append(param, QMI_WMS_PARAM_RAW_READ_MESSAGE_ID,
+			       sizeof(read_message), &read_message))
+		goto err;
+	if (!qmi_param_append_uint8(param, QMI_WMS_PARAM_RAW_READ_MESSAGE_MODE,
+			 QMI_WMS_MESSAGE_MODE_GSMWCDMA))
+		goto err;
+
+	if (qmi_service_send(data->wms, QMI_WMS_RAW_READ, param,
+					read_message_cb, read_data, NULL) > 0)
+		return;
+err:
+	qmi_param_free(param);
+	g_free(read_data);
+}
+
+static void get_msg_list_cb(struct qmi_result *result, void *user_data)
+{
+	struct ofono_sms *sms = user_data;
+	const struct qmi_wms_message_list *list;
+	uint16_t len;
+	uint32_t num;
+	int i;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, NULL))
+		return;
+
+	list = qmi_result_get(result, QMI_WMS_RESULT_MESSAGE_LIST, &len);
+	if (!list)
+		return;
+
+	num = GUINT32_FROM_LE(list->count);
+	if (num * sizeof(*list->message) + 4 > len) {
+		DBG("invalid length of the message list");
+		return;
+	}
+
+	DBG("found %d messages", num);
+
+	for (i = 0; i < num; i++) {
+		uint32_t index = GUINT32_FROM_LE(list->message[i].memory_index);
+
+		DBG("message %d tags %d", index, list->message[i].message_tag);
+		/* read messages, delete the message in the callback */
+		read_delete_message(sms, index);
+	}
+}
+
+static void list_messages(struct ofono_sms *sms, enum qmi_wms_message_tag tag)
+{
+	struct sms_data *data = ofono_sms_get_data(sms);
+	struct qmi_param *param;
+
+	DBG("");
+
+	param = qmi_param_new();
+	if (!param)
+		return;
+
+	if (!qmi_param_append_uint8(param,
+				QMI_WMS_PARAM_LIST_MESSAGE_STORAGE_TYPE,
+				QMI_WMS_STORAGE_TYPE_NV))
+		goto err;
+
+	if (!qmi_param_append_uint8(param, QMI_WMS_PARAM_LIST_MESSAGE_MODE,
+			       QMI_WMS_MESSAGE_MODE_GSMWCDMA))
+		goto err;
+
+	if (!qmi_param_append_uint8(param, QMI_WMS_PARAM_LIST_MESSAGE_TAG, tag))
+		goto err;
+
+	if (qmi_service_send(data->wms, QMI_WMS_GET_MSG_LIST, param,
+				get_msg_list_cb, sms, NULL) > 0)
+		return;
+err:
+	qmi_param_free(param);
+}
+
+
 static void event_notify(struct qmi_result *result, void *user_data)
 {
 	struct ofono_sms *sms = user_data;
@@ -369,6 +533,10 @@ static void set_routes_cb(struct qmi_result *result, void *user_data)
 
 	DBG("");
 
+	list_messages(sms, QMI_WMS_MESSAGE_TAG_TYPE_MT_READ);
+	list_messages(sms, QMI_WMS_MESSAGE_TAG_TYPE_MT_NOT_READ);
+	list_messages(sms, QMI_WMS_MESSAGE_TAG_TYPE_MO_SENT);
+	list_messages(sms, QMI_WMS_MESSAGE_TAG_TYPE_MO_NOT_SENT);
 	ofono_sms_register(sms);
 }
 
diff --git a/drivers/qmimodem/wms.h b/drivers/qmimodem/wms.h
index dae86c17..fde2cbc1 100644
--- a/drivers/qmimodem/wms.h
+++ b/drivers/qmimodem/wms.h
@@ -24,6 +24,8 @@
 #define QMI_WMS_SET_EVENT		1	/* Set new message conditions */
 
 #define QMI_WMS_RAW_SEND		32	/* Send a raw message */
+#define QMI_WMS_RAW_READ		34	/* Read a raw message */
+#define QMI_WMS_DELETE_MESSAGE		36	/* Delete a message */
 
 #define QMI_WMS_GET_MSG_LIST		49	/* Get list of messages from the device */
 #define QMI_WMS_SET_ROUTES		50	/* Set routes for message memory storage */
@@ -55,16 +57,61 @@ struct qmi_wms_param_message {
 } __attribute__((__packed__));
 #define QMI_WMS_RESULT_MESSAGE_ID		0x01	/* uint16 */
 
-/* Get list of messages from the device */
-#define QMI_WMS_PARAM_STORAGE_TYPE		0x01	/* uint8 */
-#define QMI_WMS_PARAM_MESSAGE_MODE		0x11	/* uint8 */
-
 #define QMI_WMS_STORAGE_TYPE_UIM		0
 #define QMI_WMS_STORAGE_TYPE_NV			1
 #define QMI_WMS_STORAGE_TYPE_UNKNOWN		2
 
 #define QMI_WMS_MESSAGE_MODE_GSMWCDMA		1
 
+#define QMI_WMS_RESULT_MESSAGE_LIST		0x01
+struct qmi_wms_message_list {
+	uint32_t count;
+	struct {
+		uint32_t memory_index;
+		uint8_t message_tag;
+	} __attribute__((__packed__)) message[0];
+} __attribute__((__packed__));
+
+enum qmi_wms_message_tag {
+	QMI_WMS_MESSAGE_TAG_TYPE_MT_READ     = 0x00,
+	QMI_WMS_MESSAGE_TAG_TYPE_MT_NOT_READ = 0x01,
+	QMI_WMS_MESSAGE_TAG_TYPE_MO_SENT     = 0x02,
+	QMI_WMS_MESSAGE_TAG_TYPE_MO_NOT_SENT = 0x03
+};
+
+/* read a message */
+enum qmi_wms_param_read_message_req {
+	QMI_WMS_PARAM_RAW_READ_MESSAGE_ID	= 0x01,
+	QMI_WMS_PARAM_RAW_READ_MESSAGE_MODE	= 0x10,
+};
+enum qmi_wms_param_read_message_res {
+	QMI_WMS_PARAM_RAW_READ_MESSAGE	= 0x01,
+};
+struct qmi_wms_raw_message {
+	uint8_t message_tag;
+	uint8_t message_format;
+	uint16_t count;
+	uint8_t data[0];
+} __attribute__((__packed__));
+
+/* delete a message */
+enum qmi_wms_params_delete_req {
+	QMI_WMS_PARAM_DELETE_STORAGE		= 0x01,
+	QMI_WMS_PARAM_DELETE_MESSAGE_INDEX	= 0x10,
+	QMI_WMS_PARAM_DELETE_MESSAGE_MODE	= 0x12,
+};
+
+/* Get list of messages from the device response */
+enum qmi_wms_params_list_message_req {
+	QMI_WMS_PARAM_LIST_MESSAGE_STORAGE_TYPE	= 0x01,
+	QMI_WMS_PARAM_LIST_MESSAGE_TAG		= 0x11,
+	QMI_WMS_PARAM_LIST_MESSAGE_MODE		= 0x12,
+};
+
+enum qmi_wms_params_list_message_res {
+	QMI_WMS_PARAM_LIST_MESSAGE_LIST		= 0x01,
+};
+
 /* Get routes for message memory storage */
 #define QMI_WMS_RESULT_ROUTE_LIST		0x01
 #define QMI_WMS_PARAM_ROUTE_LIST		0x01
-- 
2.13.0

Re: [PATCH 2/2] qmimodem: receive & delete sms from NV storage on 'Powered'

[Denis Kenzior]

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

Hi Alexander,

On 05/16/2017 10:49 AM, Alexander Couzens wrote:
> When the modem is powered download all message from the storage
> and delete them afterwards. This is required as certain modems
> (e.g. Gobi 2000) won't receive any further messages and generate
> a SMPP RP-Error 'Protocol error, unspecific'. Even those messages
> should not saved in the ME.
>
> This should also the case for newer modems, like the EC20, but they
> have a much bigger storage as it doesn't got noticed.
> Storage capacity: EC20: 255 msg, Gobi2000: 23 msg.
> ---
>  drivers/qmimodem/sms.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/qmimodem/wms.h |  55 ++++++++++++++--
>  2 files changed, 219 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/qmimodem/sms.c b/drivers/qmimodem/sms.c
> index 43bf22d1..4df65962 100644
> --- a/drivers/qmimodem/sms.c
> +++ b/drivers/qmimodem/sms.c
> @@ -334,6 +334,170 @@ error:
>  	g_free(cbd);
>  }
>
> +static void delete_message(struct ofono_sms *sms, uint32_t message_id)
> +{
> +	struct qmi_param *param = qmi_param_new();
> +	struct sms_data *data = ofono_sms_get_data(sms);
> +
> +	if (!param)
> +		return;
> +
> +	DBG("remove message %08x", message_id);
> +
> +	qmi_param_append_uint8(param, QMI_WMS_PARAM_DELETE_STORAGE,
> +			       QMI_WMS_STORAGE_TYPE_NV);
> +	qmi_param_append_uint32(param, QMI_WMS_PARAM_DELETE_MESSAGE_INDEX,
> +			       message_id);
> +	qmi_param_append_uint8(param, QMI_WMS_PARAM_DELETE_MESSAGE_MODE,
> +			 QMI_WMS_MESSAGE_MODE_GSMWCDMA);
> +
> +	if (qmi_service_send(data->wms, QMI_WMS_DELETE_MESSAGE, param,
> +					NULL, NULL, NULL) > 0)
> +		return;

Okay, but pointless.  If you don't care about the return value might as 
well just don't wrap it in the 'if'

> +}
> +
> +struct read_message_data {
> +	struct ofono_sms *sms;
> +	uint32_t message_id;
> +};
> +
> +static void read_message_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct read_message_data *read_data = user_data;
> +	const struct qmi_wms_raw_message *message;
> +	uint16_t len;
> +	uint16_t plen;
> +
> +	message = qmi_result_get(result, QMI_WMS_PARAM_RAW_READ_MESSAGE, &len);
> +	if (!message)
> +		return;

You're leaking read_data here and a few other places below.  Please 
learn to use the destroy callback, that is what it is there for.

> +
> +	/* -1 when the payload length is 0 */
> +	if (sizeof(*message) - 1 > len)
> +		return;
> +

???

> +	plen = GUINT16_FROM_LE(message->count);
> +	if (plen + 4 > len)
> +		return;
> +
> +	switch (message->message_tag) {
> +	case QMI_WMS_MESSAGE_TAG_TYPE_MT_NOT_READ:
> +	case QMI_WMS_MESSAGE_TAG_TYPE_MT_READ:
> +		ofono_sms_deliver_notify(read_data->sms,
> +						message->data,
> +						plen,
> +						TPDU_UNKNOWN_WITH_SC);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	delete_message(read_data->sms, read_data->message_id);
> +	g_free(read_data);
> +}
> +
> +static void read_delete_message(struct ofono_sms *sms, uint32_t message_id)
> +{
> +	struct sms_data *data = ofono_sms_get_data(sms);
> +	struct qmi_wms_result_new_msg_notify read_message;
> +	struct qmi_param *param = NULL;
> +	struct read_message_data *read_data = NULL;
> +
> +	DBG("");
> +
> +	read_data = g_try_new0(struct read_message_data, 1);
> +	if (!read_data)
> +		return;

Not our style.  See doc/coding-style.txt item M1.  Also, we have deemed 
it acceptable to use g_new0 for small allocations, so just make your 
life simple.

> +	read_data->sms = sms;
> +	read_data->message_id = message_id;
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto err;
> +
> +	read_message.storage_index = GUINT32_TO_LE(message_id);
> +	read_message.storage_type = QMI_WMS_STORAGE_TYPE_NV;
> +
> +	if (!qmi_param_append(param, QMI_WMS_PARAM_RAW_READ_MESSAGE_ID,
> +			       sizeof(read_message), &read_message))
> +		goto err;

Not our style

> +	if (!qmi_param_append_uint8(param, QMI_WMS_PARAM_RAW_READ_MESSAGE_MODE,
> +			 QMI_WMS_MESSAGE_MODE_GSMWCDMA))
> +		goto err;
> +
> +	if (qmi_service_send(data->wms, QMI_WMS_RAW_READ, param,
> +					read_message_cb, read_data, NULL) > 0)
> +		return;

As above

> +err:
> +	qmi_param_free(param);
> +	g_free(read_data);
> +}
> +
> +static void get_msg_list_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct ofono_sms *sms = user_data;
> +	const struct qmi_wms_message_list *list;
> +	uint16_t len;
> +	uint32_t num;
> +	int i;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, NULL))
> +		return;
> +
> +	list = qmi_result_get(result, QMI_WMS_RESULT_MESSAGE_LIST, &len);
> +	if (!list)
> +		return;
> +
> +	num = GUINT32_FROM_LE(list->count);
> +	if (num * sizeof(*list->message) + 4 > len) {
> +		DBG("invalid length of the message list");
> +		return;
> +	}
> +
> +	DBG("found %d messages", num);
> +
> +	for (i = 0; i < num; i++) {
> +		uint32_t index = GUINT32_FROM_LE(list->message[i].memory_index);
> +
> +		DBG("message %d tags %d", index, list->message[i].message_tag);
> +		/* read messages, delete the message in the callback */
> +		read_delete_message(sms, index);
> +	}
> +}
> +
> +static void list_messages(struct ofono_sms *sms, enum qmi_wms_message_tag tag)
> +{
> +	struct sms_data *data = ofono_sms_get_data(sms);
> +	struct qmi_param *param;
> +
> +	DBG("");
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		return;
> +
> +	if (!qmi_param_append_uint8(param,
> +				QMI_WMS_PARAM_LIST_MESSAGE_STORAGE_TYPE,
> +				QMI_WMS_STORAGE_TYPE_NV))
> +		goto err;
> +
> +	if (!qmi_param_append_uint8(param, QMI_WMS_PARAM_LIST_MESSAGE_MODE,
> +			       QMI_WMS_MESSAGE_MODE_GSMWCDMA))
> +		goto err;
> +
> +	if (!qmi_param_append_uint8(param, QMI_WMS_PARAM_LIST_MESSAGE_TAG, tag))
> +		goto err;
> +
> +	if (qmi_service_send(data->wms, QMI_WMS_GET_MSG_LIST, param,
> +				get_msg_list_cb, sms, NULL) > 0)
> +		return;
> +err:
> +	qmi_param_free(param);
> +}
> +
> +
>  static void event_notify(struct qmi_result *result, void *user_data)
>  {
>  	struct ofono_sms *sms = user_data;
> @@ -369,6 +533,10 @@ static void set_routes_cb(struct qmi_result *result, void *user_data)
>
>  	DBG("");
>
> +	list_messages(sms, QMI_WMS_MESSAGE_TAG_TYPE_MT_READ);
> +	list_messages(sms, QMI_WMS_MESSAGE_TAG_TYPE_MT_NOT_READ);
> +	list_messages(sms, QMI_WMS_MESSAGE_TAG_TYPE_MO_SENT);
> +	list_messages(sms, QMI_WMS_MESSAGE_TAG_TYPE_MO_NOT_SENT);
>  	ofono_sms_register(sms);
>  }
>
> diff --git a/drivers/qmimodem/wms.h b/drivers/qmimodem/wms.h
> index dae86c17..fde2cbc1 100644
> --- a/drivers/qmimodem/wms.h
> +++ b/drivers/qmimodem/wms.h
> @@ -24,6 +24,8 @@
>  #define QMI_WMS_SET_EVENT		1	/* Set new message conditions */
>
>  #define QMI_WMS_RAW_SEND		32	/* Send a raw message */
> +#define QMI_WMS_RAW_READ		34	/* Read a raw message */
> +#define QMI_WMS_DELETE_MESSAGE		36	/* Delete a message */
>
>  #define QMI_WMS_GET_MSG_LIST		49	/* Get list of messages from the device */
>  #define QMI_WMS_SET_ROUTES		50	/* Set routes for message memory storage */
> @@ -55,16 +57,61 @@ struct qmi_wms_param_message {
>  } __attribute__((__packed__));
>  #define QMI_WMS_RESULT_MESSAGE_ID		0x01	/* uint16 */
>
> -/* Get list of messages from the device */
> -#define QMI_WMS_PARAM_STORAGE_TYPE		0x01	/* uint8 */
> -#define QMI_WMS_PARAM_MESSAGE_MODE		0x11	/* uint8 */
> -
>  #define QMI_WMS_STORAGE_TYPE_UIM		0
>  #define QMI_WMS_STORAGE_TYPE_NV			1
>  #define QMI_WMS_STORAGE_TYPE_UNKNOWN		2
>
>  #define QMI_WMS_MESSAGE_MODE_GSMWCDMA		1
>
> +#define QMI_WMS_RESULT_MESSAGE_LIST		0x01
> +struct qmi_wms_message_list {
> +	uint32_t count;
> +	struct {
> +		uint32_t memory_index;
> +		uint8_t message_tag;
> +	} __attribute__((__packed__)) message[0];
> +} __attribute__((__packed__));
> +
> +enum qmi_wms_message_tag {
> +	QMI_WMS_MESSAGE_TAG_TYPE_MT_READ     = 0x00,
> +	QMI_WMS_MESSAGE_TAG_TYPE_MT_NOT_READ = 0x01,
> +	QMI_WMS_MESSAGE_TAG_TYPE_MO_SENT     = 0x02,
> +	QMI_WMS_MESSAGE_TAG_TYPE_MO_NOT_SENT = 0x03
> +};
> +
> +/* read a message */
> +enum qmi_wms_param_read_message_req {
> +	QMI_WMS_PARAM_RAW_READ_MESSAGE_ID	= 0x01,
> +	QMI_WMS_PARAM_RAW_READ_MESSAGE_MODE	= 0x10,
> +};
> +enum qmi_wms_param_read_message_res {
> +	QMI_WMS_PARAM_RAW_READ_MESSAGE	= 0x01,
> +};
> +struct qmi_wms_raw_message {
> +	uint8_t message_tag;

msg_tag

> +	uint8_t message_format;

msg_format

> +	uint16_t count;

Should this not be msg_length to be consistent with 
qmi_wms_param_message and qmi_wms_result_message

> +	uint8_t data[0];
> +} __attribute__((__packed__));
> +
> +/* delete a message */
> +enum qmi_wms_params_delete_req {
> +	QMI_WMS_PARAM_DELETE_STORAGE		= 0x01,
> +	QMI_WMS_PARAM_DELETE_MESSAGE_INDEX	= 0x10,
> +	QMI_WMS_PARAM_DELETE_MESSAGE_MODE	= 0x12,
> +};
> +
> +/* Get list of messages from the device response */
> +enum qmi_wms_params_list_message_req {
> +	QMI_WMS_PARAM_LIST_MESSAGE_STORAGE_TYPE	= 0x01,
> +	QMI_WMS_PARAM_LIST_MESSAGE_TAG		= 0x11,
> +	QMI_WMS_PARAM_LIST_MESSAGE_MODE		= 0x12,
> +};
> +
> +enum qmi_wms_params_list_message_res {
> +	QMI_WMS_PARAM_LIST_MESSAGE_LIST		= 0x01,
> +};
> +
>  /* Get routes for message memory storage */
>  #define QMI_WMS_RESULT_ROUTE_LIST		0x01
>  #define QMI_WMS_PARAM_ROUTE_LIST		0x01
>

Regards,
-Denis

Re: [PATCH 1/2] smsutil: sms_decode() allow to pass sms with unknown TPDU size

[Denis Kenzior]

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

Hi Alexander,

On 05/16/2017 10:49 AM, Alexander Couzens wrote:
> Certain modems (QMI based) only return a PDU length, but without
> the length of the TPDU.

It looks like the current QMI_WMS_RESULT_NEW_MSG_NOTIFY assumes that the 
message doesn't have an SMSC preceeding it.  Or is it that we are not 
parsing some other parameters?

> Without passing this information over ofono_sms_deliver_notify() down
> to sms_decode(), sms_decode() will fail, as it try to interpret the
> SC header as PDU header.

How do we know when a PDU contains the SC address and when it doesn't?

> ---
>
> I'm not quite sure this is the best way to implement this.
>
> a) adding a flag to ofono_sms_deliver_notify() would break the api

No, this is not the way to do it

> b) export sms_decode_address_field() which is part of smsutil.h would mean
>    using this function only to get the length of TPDU and throw away the
>    result (address), which is later called a second time.

In-tree drivers can freely utilize core utilities such as smsutil and 
stkutil.  So this isn't really a problem.  Since the QMI API is quite 
dumb, then parsing the SC address field twice is probably the least evil 
way to do this.

> c) add another function to cover this case is also bloated
>
>
>  include/sms.h | 2 ++
>  src/smsutil.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/sms.h b/include/sms.h
> index e9260561..602042d2 100644
> --- a/include/sms.h
> +++ b/include/sms.h
> @@ -60,6 +60,8 @@ struct ofono_sms_driver {
>  				ofono_sms_bearer_set_cb_t, void *data);
>  };
>
> +/* unknown tpdu length, but contains a SC header */
> +#define TPDU_UNKNOWN_WITH_SC -2


Nak.  This couples together the core <-> driver API with the utility 
functions.  They were always independent of each other and should remain 
this way.

>  void ofono_sms_deliver_notify(struct ofono_sms *sms, const unsigned char *pdu,
>  				int len, int tpdu_len);
>  void ofono_sms_status_notify(struct ofono_sms *sms, const unsigned char *pdu,
> diff --git a/src/smsutil.c b/src/smsutil.c
> index 24dcfaa7..d4b4a0d4 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -33,6 +33,7 @@
>  #include <unistd.h>
>
>  #include <glib.h>
> +#include <ofono/sms.h>
>
>  #include "util.h"
>  #include "storage.h"
> @@ -1545,7 +1546,12 @@ gboolean sms_decode(const unsigned char *pdu, int len, gboolean outgoing,
>
>  	memset(out, 0, sizeof(*out));
>
> -	if (tpdu_len < len) {
> +	if (tpdu_len == TPDU_UNKNOWN_WITH_SC) {
> +		/* unknown TPDU length but prefixed with SC */
> +		sms_decode_address_field(pdu, len, &offset, TRUE,
> +				&out->sc_addr);

Why don't you actually check the return value ?

> +		tpdu_len = len - offset;
> +	} else if (tpdu_len < len) {
>  		if (!sms_decode_address_field(pdu, len, &offset,
>  						TRUE, &out->sc_addr))
>  			return FALSE;
>

Regards,
-Denis

Re: [PATCH 1/2] smsutil: sms_decode() allow to pass sms with unknown TPDU size

[Alexander Couzens]

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

Hi Denis,

> Hi Alexander,
> 
> On 05/16/2017 10:49 AM, Alexander Couzens wrote:
> > Certain modems (QMI based) only return a PDU length, but without
> > the length of the TPDU.  
> 
> It looks like the current QMI_WMS_RESULT_NEW_MSG_NOTIFY assumes that
> the message doesn't have an SMSC preceeding it.  Or is it that we are
> not parsing some other parameters?

The nice thing here is:
When receiving a SMS PDU over the notify callback, it doesn't contain a
SC address, but when reading it from NV it does.

> > Without passing this information over ofono_sms_deliver_notify()
> > down to sms_decode(), sms_decode() will fail, as it try to
> > interpret the SC header as PDU header.  
> 
> How do we know when a PDU contains the SC address and when it doesn't?
I don't know. Sorry, I didn't read (yet) the whole spec. 

Originally I though it would be possible and a good idea to detect SC,
but sms_decode_address_field() can exit early with success and
sms_decode() fail later when decoding. A second pass is then required.
I dropped that idea and use fixed expectation, hopefully QMI behave the
same way on different devices.

> [..]

> > b) export sms_decode_address_field() which is part of smsutil.h
> > would mean using this function only to get the length of TPDU and
> > throw away the result (address), which is later called a second
> > time.  
> 
> In-tree drivers can freely utilize core utilities such as smsutil and 
> stkutil.  So this isn't really a problem.  Since the QMI API is quite 
> dumb, then parsing the SC address field twice is probably the least
> evil way to do this.

Ok. Will do it that way.

-- 
Alexander Couzens

mail: lynxis(a)fe80.eu
jabber: lynxis(a)fe80.eu
mobile: +4915123277221
gpg: 390D CF78 8BF9 AA50 4F8F  F1E2 C29E 9DA6 A0DF 8604

[-- Attachment #2: attachment.sig --]
[-- Type: application/pgp-signature, Size: 833 bytes --]