[PATCH v1 0/2] Handles errors while receiving messages

[PATCH v1 0/2] Handles errors while receiving messages

[Ronald Tessier]

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

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 (2):
  service: remove files when unable to decode received msg
  service: handle error while receiving messages

 src/service.c |  101 +++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 80 insertions(+), 21 deletions(-)

--
1.7.4.1

[PATCH v1 1/2] service: remove files when unable to decode received msg

[Ronald Tessier]

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

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

diff --git a/src/service.c b/src/service.c
index f148252..851db34 100644
--- a/src/service.c
+++ b/src/service.c
@@ -984,7 +984,12 @@ static gboolean load_message_from_store(const char *service_id,
 	if (mms_message_decode(pdu, len, msg) == FALSE) {
 		mms_error("Failed to decode %s", data_path);
 		munmap(pdu, len);
-		goto out;
+		g_free(state);
+
+		mms_store_meta_close(service_id, uuid, meta, FALSE);
+		mms_store_remove(service_id, uuid);
+
+		return FALSE;
 	}

 	munmap(pdu, len);
--
1.7.4.1

[PATCH v1 2/2] service: handle error while receiving messages

[Ronald Tessier]

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

---
 src/service.c |   94 ++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 74 insertions(+), 20 deletions(-)

diff --git a/src/service.c b/src/service.c
index 851db34..c3fc0c6 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;
 }

@@ -1656,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);
@@ -1798,16 +1807,42 @@ exit:
 	munmap(pdu, len);
 }

+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;
 	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)
-		goto error;
+	if (g_web_result_get_chunk(result, &chunk, &chunk_size) == FALSE) {
+		mms_error("Fail to get data chunk");
+
+		close(request->fd);
+
+		if (mms_requeue_request(request) == FALSE)
+			unlink(request->data_path);
+
+		goto exit;
+	}

 	if (chunk_size == 0) {
 		close(request->fd);
@@ -1826,26 +1861,35 @@ 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;
+
+		close(request->fd);
+		unlink(request->data_path);
+
+		goto complete;
 	}

 	return TRUE;

-error:
-	close(request->fd);
-	unlink(request->data_path);
-
 complete:
-	service = request->service;
+	if (request->status >= 300) {
+		mms_error("Fail to get data (http status = %03u)",
+							request->status);
+		if (mms_requeue_request(request) == TRUE) {
+			mms_error("retry later");
+			goto exit;
+		}
+		unlink(request->data_path);
+	}

 	if (request->result_cb != NULL)
 		request->result_cb(request);

 	mms_request_destroy(request);

-	service->current_request_id = 0;
+exit:
+	request->service->current_request_id = 0;

-	process_request_queue(service);
+	process_request_queue(request->service);

 	return FALSE;
 }
@@ -1906,8 +1950,15 @@ static guint process_request(struct mms_request *request)
 		return id;
 	}

+	mms_error("Cannot process request (request type: %d)", request->type);
+
+	if (mms_requeue_request(request) == TRUE)
+		return 0;
+
 	unlink(request->data_path);

+	mms_request_destroy(request);
+
 	return 0;
 }

@@ -1926,6 +1977,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;
@@ -1935,8 +1989,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,
@@ -2022,6 +2074,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.1

Re: [PATCH v1 1/2] service: remove files when unable to decode received msg

[Marcel Holtmann]

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

Hi Ronald,

>  src/service.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/src/service.c b/src/service.c
> index f148252..851db34 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -984,7 +984,12 @@ static gboolean load_message_from_store(const char *service_id,
>  	if (mms_message_decode(pdu, len, msg) == FALSE) {
>  		mms_error("Failed to decode %s", data_path);
>  		munmap(pdu, len);
> -		goto out;
> +		g_free(state);

are we not leaking data_path here?

> +
> +		mms_store_meta_close(service_id, uuid, meta, FALSE);
> +		mms_store_remove(service_id, uuid);
> +
> +		return FALSE;
>  	}

And why are we turning a perfect good label into something were we have
the label and also duplicated code. This is not a good idea.

Regards

Marcel

Re: [PATCH v1 2/2] service: handle error while receiving messages

[Marcel Holtmann]

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

Hi Ronald,

>  src/service.c |   94 ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 74 insertions(+), 20 deletions(-)
> 
> diff --git a/src/service.c b/src/service.c
> index 851db34..c3fc0c6 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;
>  }
> 
> @@ -1656,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);
> @@ -1798,16 +1807,42 @@ exit:
>  	munmap(pdu, len);
>  }
> 
> +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;
>  	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)
> -		goto error;
> +	if (g_web_result_get_chunk(result, &chunk, &chunk_size) == FALSE) {
> +		mms_error("Fail to get data chunk");
> +
> +		close(request->fd);
> +
> +		if (mms_requeue_request(request) == FALSE)
> +			unlink(request->data_path);
> +
> +		goto exit;
> +	}
> 
>  	if (chunk_size == 0) {
>  		close(request->fd);
> @@ -1826,26 +1861,35 @@ 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;
> +
> +		close(request->fd);
> +		unlink(request->data_path);
> +
> +		goto complete;
>  	}
> 

same here. I do not like this. Please make clean and small changes that
are not re-factoring major pieces. If re-factoring seems to be needed,
please separate them out so they can be reviewed.

Regards

Marcel