[PATCH] gprs: Setup route for mmsc if there's no mms proxy

[PATCH] gprs: Setup route for mmsc if there's no mms proxy

[Slava Monich]

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

And don't setup mms route at all if mmsc/proxy host name is not in
dotted IPv4 format.
---
 src/gprs.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 05ab499..9c643b9 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -585,13 +585,16 @@ static void pri_context_signal_settings(struct pri_context *ctx,
 				context_settings_append_ipv6);
 }
 
-static void pri_parse_proxy(struct pri_context *ctx, const char *proxy)
+static gboolean pri_parse_proxy(struct pri_context *ctx, const char *proxy)
 {
 	char *scheme, *host, *port, *path;
 
+	if (proxy[0] == 0)
+		return FALSE;
+
 	scheme = g_strdup(proxy);
 	if (scheme == NULL)
-		return;
+		return FALSE;
 
 	host = strstr(scheme, "://");
 	if (host != NULL) {
@@ -604,7 +607,7 @@ static void pri_parse_proxy(struct pri_context *ctx, const char *proxy)
 			ctx->proxy_port = 80;
 		else {
 			g_free(scheme);
-			return;
+			return FALSE;
 		}
 	} else {
 		host = scheme;
@@ -626,10 +629,16 @@ static void pri_parse_proxy(struct pri_context *ctx, const char *proxy)
 		}
 	}
 
+	if (host[0] == 0) {
+		g_free(scheme);
+		return FALSE;
+	}
+
 	g_free(ctx->proxy_host);
 	ctx->proxy_host = g_strdup(host);
 
 	g_free(scheme);
+	return TRUE;
 }
 
 static void pri_ifupdown(const char *interface, ofono_bool_t active)
@@ -715,11 +724,16 @@ static void pri_setproxy(const char *interface, const char *proxy)
 {
 	struct rtentry rt;
 	struct sockaddr_in addr;
+	in_addr_t proxy_addr;
 	int sk;
 
 	if (interface == NULL)
 		return;
 
+	proxy_addr = inet_addr(proxy);
+	if (proxy_addr == INADDR_NONE)
+		return;
+
 	sk = socket(PF_INET, SOCK_DGRAM, 0);
 	if (sk < 0)
 		return;
@@ -730,7 +744,7 @@ static void pri_setproxy(const char *interface, const char *proxy)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sin_family = AF_INET;
-	addr.sin_addr.s_addr = inet_addr(proxy);
+	addr.sin_addr.s_addr = proxy_addr;
 	memcpy(&rt.rt_dst, &addr, sizeof(rt.rt_dst));
 
 	memset(&addr, 0, sizeof(addr));
@@ -792,7 +806,8 @@ static void pri_update_mms_context_settings(struct pri_context *ctx)
 	if (ctx->message_proxy)
 		settings->ipv4->proxy = g_strdup(ctx->message_proxy);
 
-	pri_parse_proxy(ctx, ctx->message_proxy);
+	if (!pri_parse_proxy(ctx, ctx->message_proxy))
+		pri_parse_proxy(ctx, ctx->message_center);
 
 	DBG("proxy %s port %u", ctx->proxy_host, ctx->proxy_port);
 
-- 
1.8.3.2

Re: [PATCH] gprs: Setup route for mmsc if there's no mms proxy

[Marcel Holtmann]

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

Hi Slava,

> And don't setup mms route at all if mmsc/proxy host name is not in
> dotted IPv4 format.
> ---
> src/gprs.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index 05ab499..9c643b9 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -585,13 +585,16 @@ static void pri_context_signal_settings(struct pri_context *ctx,
> 				context_settings_append_ipv6);
> }
> 
> -static void pri_parse_proxy(struct pri_context *ctx, const char *proxy)
> +static gboolean pri_parse_proxy(struct pri_context *ctx, const char *proxy)
> {
> 	char *scheme, *host, *port, *path;
> 
> +	if (proxy[0] == 0)
> +		return FALSE;
> +
> 	scheme = g_strdup(proxy);
> 	if (scheme == NULL)
> -		return;
> +		return FALSE;
> 
> 	host = strstr(scheme, "://");
> 	if (host != NULL) {
> @@ -604,7 +607,7 @@ static void pri_parse_proxy(struct pri_context *ctx, const char *proxy)
> 			ctx->proxy_port = 80;
> 		else {
> 			g_free(scheme);
> -			return;
> +			return FALSE;
> 		}
> 	} else {
> 		host = scheme;
> @@ -626,10 +629,16 @@ static void pri_parse_proxy(struct pri_context *ctx, const char *proxy)
> 		}
> 	}
> 
> +	if (host[0] == 0) {
> +		g_free(scheme);
> +		return FALSE;
> +	}
> +
> 	g_free(ctx->proxy_host);
> 	ctx->proxy_host = g_strdup(host);
> 
> 	g_free(scheme);
> +	return TRUE;
> }

There is a single call instance for pri_parse_proxy. Why are we not checking right there if the ctx->message_proxy is something that is worth while parsing.

> static void pri_ifupdown(const char *interface, ofono_bool_t active)
> @@ -715,11 +724,16 @@ static void pri_setproxy(const char *interface, const char *proxy)
> {
> 	struct rtentry rt;
> 	struct sockaddr_in addr;
> +	in_addr_t proxy_addr;
> 	int sk;
> 
> 	if (interface == NULL)
> 		return;
> 
> +	proxy_addr = inet_addr(proxy);
> +	if (proxy_addr == INADDR_NONE)
> +		return;
> +
> 	sk = socket(PF_INET, SOCK_DGRAM, 0);
> 	if (sk < 0)
> 		return;
> @@ -730,7 +744,7 @@ static void pri_setproxy(const char *interface, const char *proxy)
> 
> 	memset(&addr, 0, sizeof(addr));
> 	addr.sin_family = AF_INET;
> -	addr.sin_addr.s_addr = inet_addr(proxy);
> +	addr.sin_addr.s_addr = proxy_addr;
> 	memcpy(&rt.rt_dst, &addr, sizeof(rt.rt_dst));
> 
> 	memset(&addr, 0, sizeof(addr));

This extra handling seems unneeded. We do not call pri_setproxy when ctx->proxy_host == NULL. So what should happen is to ensure that ctx->proxy_host is set to NULL. Then no change here is needed.

> @@ -792,7 +806,8 @@ static void pri_update_mms_context_settings(struct pri_context *ctx)
> 	if (ctx->message_proxy)
> 		settings->ipv4->proxy = g_strdup(ctx->message_proxy);
> 
> -	pri_parse_proxy(ctx, ctx->message_proxy);
> +	if (!pri_parse_proxy(ctx, ctx->message_proxy))
> +		pri_parse_proxy(ctx, ctx->message_center);

What is this change doing? Parsing the message center into ctx->proxy_host makes no sense to me.

Regards

Marcel

Re: [PATCH] gprs: Setup route for mmsc if there's no mms proxy

[Slava Monich]

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


> And don't setup mms route at all if mmsc/proxy host name is not in
> dotted IPv4 format.
> ---
>   src/gprs.c | 25 ++++++++++++++++++++-----
>   1 file changed, 20 insertions(+), 5 deletions(-)

Sorry, somehow I missed the reply to this patch and just accidentally 
found it (the reply) at mail-archive.com.

This patch has to do the the case when no MessageProxy is provided for 
the MMS context. In that case ofono would set up host route for 
255.255.255.255 (INADDR_NONE returned by inet_addr for the empty string) 
for the MMS interface when it comes up.

With these changes the whole thing has a chance to work if MessageCenter 
URL contains IP address in the host part. In any case, configuring host 
route for 255.255.255.255 doesn't make any sense.

Regards,
-Slava