[PATCH v2 00/10] mmsd: error management

[PATCH v2 00/10] mmsd: error management

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

Hi,

This series concerns mmsd for ofono mailing list.

This is essentially a rework of Ronald's patches.

Was:
> Hi,

> These patches concern mmsd and are related to error handling while
> receiving message.
> The main idea is to try to recover error internally when possible and
> drop messages that are not decodable.


Ronald Tessier (1):
  service: remove files when unable to decode received msg

Sébastien Bianti (9):
  service: add debug prints
  service: remove useless variable
  service: add attempts counter variable
  service: move destroy request into process_request
  service: refactoring file closure
  service: callbacks should return a boolean
  service: refactoring error code
  service: add requeue functionality
  service: bearer activity needs to be checked

 src/service.c |  134 ++++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 90 insertions(+), 44 deletions(-)

-- 
1.7.4.4

[PATCH v2 01/10] service: remove files when unable to decode received msg

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

From: Ronald Tessier <ronald.tessier@linux.intel.com>

---
 src/service.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/src/service.c b/src/service.c
index f148252..593adcb 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1019,6 +1019,9 @@ out:
 
 	mms_store_meta_close(service_id, uuid, meta, FALSE);
 
+	if (success == FALSE)
+		mms_store_remove(service_id, uuid);
+
 	return success;
 }
 
-- 
1.7.4.4

[PATCH v2 02/10] service: add debug prints

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

---
 src/service.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/src/service.c b/src/service.c
index 593adcb..6041d6d 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1804,8 +1804,10 @@ static gboolean web_get_cb(GWebResult *result, gpointer user_data)
 	struct mms_service *service;
 	const guint8 *chunk;
 
-	if (g_web_result_get_chunk(result, &chunk, &chunk_size) == FALSE)
+	if (g_web_result_get_chunk(result, &chunk, &chunk_size) == FALSE) {
+		mms_error("Fail to get data chunk");
 		goto error;
+	}
 
 	if (chunk_size == 0) {
 		close(request->fd);
@@ -1904,6 +1906,8 @@ static guint process_request(struct mms_request *request)
 		return id;
 	}
 
+	mms_error("Cannot process request (request type: %d)", request->type);
+
 	unlink(request->data_path);
 
 	return 0;
@@ -2020,6 +2024,8 @@ error:
 
 out:
 	mms_message_free(msg);
+
+	mms_error("Failed to handle incoming notification");
 }
 
 void mms_service_bearer_notify(struct mms_service *service, mms_bool_t active,
-- 
1.7.4.4

[PATCH v2 03/10] service: remove useless variable

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

---
 src/service.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/service.c b/src/service.c
index 6041d6d..7969988 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1801,7 +1801,6 @@ static gboolean web_get_cb(GWebResult *result, gpointer user_data)
 	gsize written;
 	gsize chunk_size;
 	struct mms_request *request = user_data;
-	struct mms_service *service;
 	const guint8 *chunk;
 
 	if (g_web_result_get_chunk(result, &chunk, &chunk_size) == FALSE) {
@@ -1836,16 +1835,14 @@ error:
 	unlink(request->data_path);
 
 complete:
-	service = request->service;
-
 	if (request->result_cb != NULL)
 		request->result_cb(request);
 
 	mms_request_destroy(request);
 
-	service->current_request_id = 0;
+	request->service->current_request_id = 0;
 
-	process_request_queue(service);
+	process_request_queue(request->service);
 
 	return FALSE;
 }
-- 
1.7.4.4

[PATCH v2 04/10] service: add attempts counter variable

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

---
 src/service.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/src/service.c b/src/service.c
index 7969988..a4f236b 100644
--- a/src/service.c
+++ b/src/service.c
@@ -55,6 +55,7 @@
 #define CONTENT_TYPE_APP_SMIL "Content-Type: \"application/smil\";charset=utf-8"
 
 #define MAX_ATTACHMENTS_NUMBER 25
+#define MAX_ATTEMPTS 3
 
 #define uninitialized_var(x) x = x
 
@@ -98,6 +99,7 @@ struct mms_request {
 	gsize data_size;
 	int fd;
 	guint16 status;
+	guint16 attempt;
 	struct mms_service *service;
 	mms_request_result_cb_t result_cb;
 	struct mms_message *msg;
@@ -526,6 +528,8 @@ static struct mms_request *create_request(enum mms_request_type type,
 
 	request->status = 0;
 
+	request->attempt = 0;
+
 	return request;
 }
 
-- 
1.7.4.4

[PATCH v2 05/10] service: move destroy request into process_request

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

To make it possible to be re-queued
---
 src/service.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/service.c b/src/service.c
index a4f236b..ee06dca 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1911,6 +1911,8 @@ static guint process_request(struct mms_request *request)
 
 	unlink(request->data_path);
 
+	mms_request_destroy(request);
+
 	return 0;
 }
 
@@ -1938,8 +1940,6 @@ static void process_request_queue(struct mms_service *service)
 		service->current_request_id = process_request(request);
 		if (service->current_request_id > 0)
 			return;
-
-		mms_request_destroy(request);
 	}
 
 	service->bearer_timeout = g_timeout_add_seconds(BEARER_IDLE_TIMEOUT,
-- 
1.7.4.4

[PATCH v2 06/10] service: refactoring file closure

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

---
 src/service.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/service.c b/src/service.c
index ee06dca..1be0386 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1813,8 +1813,6 @@ static gboolean web_get_cb(GWebResult *result, gpointer user_data)
 	}
 
 	if (chunk_size == 0) {
-		close(request->fd);
-
 		request->status = g_web_result_get_status(result);
 
 		DBG("status: %03u", request->status);
@@ -1835,10 +1833,11 @@ static gboolean web_get_cb(GWebResult *result, gpointer user_data)
 	return TRUE;
 
 error:
-	close(request->fd);
 	unlink(request->data_path);
 
 complete:
+	close(request->fd);
+
 	if (request->result_cb != NULL)
 		request->result_cb(request);
 
-- 
1.7.4.4

[PATCH v2 07/10] service: callbacks should return a boolean

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

To decide whether retry or not.
---
 src/service.c |   41 +++++++++++++++++++++++------------------
 1 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/src/service.c b/src/service.c
index 1be0386..f8ad05f 100644
--- a/src/service.c
+++ b/src/service.c
@@ -68,7 +68,7 @@ static const char *sep_chars = "()<>@,;:\\\"/[]?={} \t";
 
 struct mms_request;
 
-typedef void (*mms_request_result_cb_t) (struct mms_request *request);
+typedef gboolean (*mms_request_result_cb_t) (struct mms_request *request);
 
 struct mms_service {
 	gint refcount;
@@ -581,7 +581,7 @@ static inline char *create_transaction_id(void)
 					"0123456789ABCDEF0123456789ABCDEF");
 }
 
-static void result_request_send_conf(struct mms_request *request)
+static gboolean result_request_send_conf(struct mms_request *request)
 {
 	struct mms_message *msg;
 	struct mms_service *service = request->service;
@@ -592,15 +592,15 @@ static void result_request_send_conf(struct mms_request *request)
 	char *path;
 
 	if (request->status != 200)
-		return;
+		return FALSE;
 
 	msg = g_try_new0(struct mms_message, 1);
 	if (msg == NULL)
-		return;
+		return FALSE;
 
 	if (mmap_file(request->data_path, &pdu, &len) == FALSE) {
 		mms_message_free(msg);
-		return;
+		return FALSE;
 	}
 
 	if (mms_message_decode(pdu, len, msg) == FALSE) {
@@ -610,7 +610,7 @@ static void result_request_send_conf(struct mms_request *request)
 
 		mms_message_free(msg);
 
-		return;
+		return FALSE;
 	}
 
 	mms_debug("response status : %d", msg->sc.rsp_status);
@@ -622,13 +622,13 @@ static void result_request_send_conf(struct mms_request *request)
 	unlink(request->data_path);
 
 	if (request->msg == NULL)
-		return;
+		return FALSE;
 
 	uuid = request->msg->uuid;
 
 	meta = mms_store_meta_open(service->identity, uuid);
 	if (meta == NULL)
-		return;
+		return FALSE;
 
 	g_key_file_set_string(meta, "info", "state", "sent");
 
@@ -639,6 +639,8 @@ static void result_request_send_conf(struct mms_request *request)
 	emit_msg_status_changed(path, "sent");
 
 	g_free(path);
+
+	return TRUE;
 }
 
 static void append_message(const char *path, const struct mms_service *service,
@@ -1029,8 +1031,8 @@ out:
 	return success;
 }
 
-static void result_request_retrieve_conf(struct mms_request *request);
-static void result_request_notify_resp(struct mms_request *request);
+static gboolean result_request_retrieve_conf(struct mms_request *request);
+static gboolean result_request_notify_resp(struct mms_request *request);
 
 static struct mms_request *build_notify_resp_ind(struct mms_service *service,
 					enum mms_message_notify_status status,
@@ -1693,7 +1695,7 @@ static gboolean bearer_idle_timeout(gpointer user_data)
 	return FALSE;
 }
 
-static void result_request_notify_resp(struct mms_request *request)
+static gboolean result_request_notify_resp(struct mms_request *request)
 {
 	struct mms_message *msg;
 	GKeyFile *meta;
@@ -1703,17 +1705,17 @@ static void result_request_notify_resp(struct mms_request *request)
 	if (request->status != 200) {
 		mms_error("POST m.notify.resp.ind failed with status %d",
 						request->status);
-		return;
+		return FALSE;
 	}
 
 	if (request->msg == NULL)
-		return;
+		return FALSE;
 
 	msg = mms_request_steal_message(request);
 
 	if (mms_message_register(request->service, msg) != 0) {
 		mms_message_free(msg);
-		return;
+		return FALSE;
 	}
 
 	emit_message_added(request->service, msg);
@@ -1721,15 +1723,17 @@ static void result_request_notify_resp(struct mms_request *request)
 	meta = mms_store_meta_open(request->service->identity,
 					msg->uuid);
 	if (meta == NULL)
-		return;
+		return FALSE;
 
 	g_key_file_set_string(meta, "info", "state", "received");
 
 	mms_store_meta_close(request->service->identity,
 				msg->uuid, meta, TRUE);
+
+	return TRUE;
 }
 
-static void result_request_retrieve_conf(struct mms_request *request)
+static gboolean result_request_retrieve_conf(struct mms_request *request)
 {
 	struct mms_message *msg;
 	struct mms_service *service = request->service;
@@ -1741,10 +1745,10 @@ static void result_request_retrieve_conf(struct mms_request *request)
 	gboolean decode_success;
 
 	if (request->status != 200)
-		return;
+		return FALSE;
 
 	if (mmap_file(request->data_path, &pdu, &len) == FALSE)
-		return;
+		return FALSE;
 
 	uuid = mms_store_file(service->identity, request->data_path);
 	if (uuid == NULL)
@@ -1798,6 +1802,7 @@ error:
 
 exit:
 	munmap(pdu, len);
+	return TRUE;
 }
 
 static gboolean web_get_cb(GWebResult *result, gpointer user_data)
-- 
1.7.4.4

[PATCH v2 08/10] service: refactoring error code

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

---
 src/service.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/service.c b/src/service.c
index f8ad05f..de92487 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1814,7 +1814,7 @@ static gboolean web_get_cb(GWebResult *result, gpointer user_data)
 
 	if (g_web_result_get_chunk(result, &chunk, &chunk_size) == FALSE) {
 		mms_error("Fail to get data chunk");
-		goto error;
+		goto complete;
 	}
 
 	if (chunk_size == 0) {
@@ -1832,21 +1832,21 @@ static gboolean web_get_cb(GWebResult *result, gpointer user_data)
 
 	if (written != chunk_size) {
 		mms_error("only %zd/%zd bytes written\n", written, chunk_size);
-		goto error;
+		goto complete;
 	}
 
 	return TRUE;
 
-error:
-	unlink(request->data_path);
-
 complete:
 	close(request->fd);
 
-	if (request->result_cb != NULL)
-		request->result_cb(request);
-
-	mms_request_destroy(request);
+	if (request->result_cb == NULL || request->result_cb(request) == TRUE)
+		mms_request_destroy(request);
+	else {
+		mms_error("Fail to get data (http status = %03u)",
+							request->status);
+		unlink(request->data_path);
+	}
 
 	request->service->current_request_id = 0;
 
-- 
1.7.4.4

[PATCH v2 09/10] service: add requeue functionality

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

---
 src/service.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/src/service.c b/src/service.c
index de92487..01d9886 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1805,6 +1805,25 @@ exit:
 	return TRUE;
 }
 
+static gboolean mms_requeue_request(struct mms_request *request)
+{
+	request->attempt += 1;
+
+	if (request->attempt == MAX_ATTEMPTS)
+		return FALSE;
+
+	if (request->type == MMS_REQUEST_TYPE_GET) {
+		request->fd = open(request->data_path, O_WRONLY | O_TRUNC,
+							S_IWUSR | S_IRUSR);
+		if (request->fd < 0)
+			return FALSE;
+	}
+
+	g_queue_push_tail(request->service->request_queue, request);
+
+	return TRUE;
+}
+
 static gboolean web_get_cb(GWebResult *result, gpointer user_data)
 {
 	gsize written;
@@ -1845,7 +1864,12 @@ complete:
 	else {
 		mms_error("Fail to get data (http status = %03u)",
 							request->status);
-		unlink(request->data_path);
+		if (mms_requeue_request(request) == TRUE)
+			mms_error("retry later");
+		else {
+			unlink(request->data_path);
+			mms_request_destroy(request);
+		}
 	}
 
 	request->service->current_request_id = 0;
-- 
1.7.4.4

[PATCH v2 10/10] service: bearer activity needs to be checked

[=?unknown-8bit?q?S=C3=A9bastien?= Bianti]

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

Because failure and requeing reason can be network lost, and we should
wait for recovery.
---
 src/service.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/service.c b/src/service.c
index 01d9886..550f193 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1660,17 +1660,22 @@ int mms_service_set_bearer_handler(struct mms_service *service,
 	return 0;
 }
 
-static void deactivate_bearer(struct mms_service *service)
+static inline gboolean bearer_is_active(struct mms_service *service)
 {
-	DBG("service %p", service);
-
 	if (service->bearer_setup == TRUE)
-		return;
-
-	if (service->bearer_active == FALSE)
-		return;
+		return FALSE;
 
 	if (service->bearer_handler == NULL)
+		return FALSE;
+
+	return service->bearer_active;
+}
+
+static void deactivate_bearer(struct mms_service *service)
+{
+	DBG("service %p", service);
+
+	if (bearer_is_active(service) == FALSE)
 		return;
 
 	DBG("service %p", service);
@@ -1959,6 +1964,9 @@ static void process_request_queue(struct mms_service *service)
 		return;
 
 	while (1) {
+		if (bearer_is_active(service) == FALSE)
+			return;
+
 		request = g_queue_pop_head(service->request_queue);
 		if (request == NULL)
 			break;
-- 
1.7.4.4

Re: [PATCH v2 00/10] mmsd: error management

[Ronald Tessier]

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

Hi all,

On 04/13/2012 06:33 PM, Sébastien Bianti wrote:
> Hi,
>
> This series concerns mmsd for ofono mailing list.
>
> This is essentially a rework of Ronald's patches.
>
> Was:
>> Hi,
>
>> These patches concern mmsd and are related to error handling while
>> receiving message.
>> The main idea is to try to recover error internally when possible and
>> drop messages that are not decodable.
>
>
> Ronald Tessier (1):
>    service: remove files when unable to decode received msg
>
> Sébastien Bianti (9):
>    service: add debug prints
>    service: remove useless variable
>    service: add attempts counter variable
>    service: move destroy request into process_request
>    service: refactoring file closure
>    service: callbacks should return a boolean
>    service: refactoring error code
>    service: add requeue functionality
>    service: bearer activity needs to be checked
>
>   src/service.c |  134 ++++++++++++++++++++++++++++++++++++++-------------------
>   1 files changed, 90 insertions(+), 44 deletions(-)
>

Please ignore this set of patches since the first patch has to be 
modified. I send an updated version.
Regards

Ronald