stemodem: Add RTNL handling to gprs-context.c
cover stemodem: Add RTNL handling to gprs-context.c
From: Sjur Brændeland <[email protected]>
This patch-set applies on top of:
"[PATCH v5] stemodem: Add RTNL functionality managing CAIF Network Interfaces."
I have broken this patch-set into three separate patches as requested.
1) Bug-fixes and renaming
2) Restructuring for creating interfaces statically
3) Introducing calls to caif_rtnl_* to actually use the network interfaces.
Regards,
Sjur
Sjur Brændeland (3):
stemodem: Fix for error handling, memleak and changed some defines
stemodem: Create network interfaces statically
stemodem: Use RTNL to create interfaces
drivers/stemodem/gprs-context.c | 222 ++++++++++++++++++++++++++------------- 1 files changed, 147 insertions(+), 75 deletions(-)
PATCH 1/3 stemodem: Fix for error handling, memleak and changed some defines
From: Sjur Brændeland <[email protected]>
* renamed MAX_LEN to IP_ADDR_LEN
* removed memory leak from unneeded strdup when parsing xml response.
* better handling of AT error responses
* reduced number of caif interfaces to 4
--- drivers/stemodem/gprs-context.c | 54 ++++++++++++++++++++++---------------- 1 files changed, 31 insertions(+), 23 deletions(-)
drivers/stemodem/gprs-context.c +31 -23
@@ -47,9 +47,9 @@#include "caif_socket.h" #include "if_caif.h" -#define MAX_CAIF_DEVICES 7 +#define MAX_CAIF_DEVICES 4#define MAX_DNS 2 -#define MAX_ELEM 20 +#define IP_ADDR_LEN 20#define AUTH_BUF_LENGTH (OFONO_GPRS_MAX_USERNAME_LENGTH + \ OFONO_GPRS_MAX_PASSWORD_LENGTH + 128)
@@ -73,13 +73,13 @@ struct conn_info {struct eppsd_response {char *current; -char ip_address[MAX_ELEM ]; -char subnet_mask[MAX_ELEM ]; -char mtu[MAX_ELEM ]; -char default_gateway[MAX_ELEM ]; -char dns_server1[MAX_ELEM ]; -char dns_server2[MAX_ELEM ]; -char p_cscf_server[MAX_ELEM ]; +char ip_address[IP_ADDR_LEN ]; +char subnet_mask[IP_ADDR_LEN ]; +char mtu[IP_ADDR_LEN ]; +char default_gateway[IP_ADDR_LEN ]; +char dns_server1[IP_ADDR_LEN ]; +char dns_server2[IP_ADDR_LEN ]; +char p_cscf_server[IP_ADDR_LEN ]; }; static void start_element_handler(GMarkupParseContext *context,
@@ -122,8 +122,8 @@ static void text_handler(GMarkupParseContext *context,struct eppsd_response *rsp = user_data;if (rsp->current) { -strncpy (rsp->current, text,MAX_ELEM ); - rsp->current[MAX_ELEM ] =0 ; +strncpy (rsp->current, text,IP_ADDR_LEN ); + rsp->current[IP_ADDR_LEN ] =0 ; } }
@@ -191,12 +191,16 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,ofono_gprs_context_cb_t cb = cbd->cb;struct ofono_gprs_context *gc = cbd->user;struct gprs_context_data *gcd =ofono_gprs_context_get_data (gc); -struct ofono_error error;struct conn_info *conn;GSList *l; -if (!ok) - goto error; +if (!ok) { +struct ofono_error error; + +decode_at_error (&error,g_at_result_final_response (result)); +cb (&error, cbd->data); +return ; + } l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(gcd->active_context),
@@ -217,9 +221,6 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult *result, } conn->cid =0 ; - -decode_at_error (&error,g_at_result_final_response (result)); -cb (&error, cbd->data);return ; error:
@@ -237,7 +238,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)GSList *l;int i;gsize length; -char *res_string; +const char *res_string;const char *dns[MAX_DNS +1 ];struct eppsd_response rsp;GMarkupParseContext *context =NULL ;
@@ -255,8 +256,15 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data) conn = l->data; -if (!ok) - goto error; +if (!ok) { +struct ofono_error error; + + conn->cid =0 ; + gcd->active_context =0 ; +decode_at_error (&error,g_at_result_final_response (result)); +cb (&error,NULL ,0 ,NULL ,NULL ,NULL ,NULL , cbd->data); +return ; + } rsp.current =NULL ; context =g_markup_parse_context_new (&parser,0 , &rsp,NULL );
@@ -266,7 +274,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data) for (i = 0; i < g_at_result_num_response_lines(result); i++) { g_at_result_iter_next(&iter, NULL); - res_string = strdup(g_at_result_iter_raw_line(&iter)); + res_string = g_at_result_iter_raw_line(&iter); length = strlen(res_string); if (!g_markup_parse_context_parse(context, res_string,
@@ -326,7 +334,6 @@ static void ste_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)struct ofono_error error; gcd->active_context =0 ; -decode_at_error (&error,g_at_result_final_response (result));cb (&error,NULL ,0 ,NULL ,NULL ,NULL ,NULL , cbd->data);return ;
@@ -389,7 +396,7 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,* Set username and password, this should be done after CGDCONT * or an error can occur. We don't bother with error checking * here -* */ +*/ snprintf (buf,sizeof (buf),"AT*EIAAUW=%d,1,\"%s\",\"%s\"" , ctx->cid, ctx->username, ctx->password);
@@ -398,6 +405,7 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,return ; error: + gcd->active_context =0 ;g_free (cbd);CALLBACK_WITH_FAILURE (cb,NULL ,0 ,NULL ,NULL ,NULL ,NULL , data);
Signature · 1 line
1.6.3.3
Re: [PATCH v5b 1/3] stemodem: Fix for error handling, memleak and changed some defines
Hi Sjur,
Quoted 7 lines
* renamed
MAX_LENtoIP_ADDR_LEN
* removed memory leak from unneeded strdup when parsing xml response.
* better handling of AT error responses
* reduced number of caif interfaces to 4--- drivers/stemodem/gprs-context.c | 54 ++++++++++++++++++++++---------------- 1 files changed, 31 insertions(+), 23 deletions(-)
patch has been applied. Thanks.
Regards
Marcel
PATCH 2/3 stemodem: Create network interfaces statically
In reply to Sjur BrændelandFrom: Sjur Brændeland <[email protected]>
Create the network interfaces statically at start up instead of dynamically for each PDP activation.
--- drivers/stemodem/gprs-context.c | 98 ++++++++++++++++++++------------------ 1 files changed, 52 insertions(+), 46 deletions(-)
drivers/stemodem/gprs-context.c +52 -46
@@ -28,6 +28,7 @@#include <string.h> #include <stdlib.h> #include <stdio.h> +#include <errno.h> #include <glib.h>
@@ -153,8 +154,7 @@ static gint conn_compare_by_cid(gconstpointer a, gconstpointer b)return 0 ; } -static struct conn_info *conn_info_create (unsigned int device, -unsigned int channel_id) +static struct conn_info *conn_info_create (unsigned int channel_id) { struct conn_info *connection = g_try_new0(struct conn_info, 1);
@@ -162,7 +162,6 @@ static struct conn_info *conn_info_create(unsigned int device,return NULL ; connection->cid =0 ; - connection->device = device; connection->channel_id = channel_id;return connection;
@@ -171,7 +170,7 @@ static struct conn_info *conn_info_create(unsigned int device,/* * Creates a new IP interface for CAIF. */ -static gboolean caif_if_create (const char *interface,unsigned int connid) +static gboolean caif_if_create (struct conn_info *conn) {return FALSE; }
@@ -179,9 +178,8 @@ static gboolean caif_if_create(const char *interface, unsigned int connid)/* * Removes IP interface for CAIF. */ -static gboolean caif_if_remove (const char *interface,unsigned int connid) +static void caif_if_remove (struct conn_info *conn) { -return FALSE; } static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
@@ -191,7 +189,7 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,ofono_gprs_context_cb_t cb = cbd->cb;struct ofono_gprs_context *gc = cbd->user;struct gprs_context_data *gcd =ofono_gprs_context_get_data (gc); -struct conn_info *conn; +struct conn_info *conn =NULL ;GSList *l;if (!ok) {
@@ -214,13 +212,8 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult *result, } conn = l->data; - -if (!caif_if_remove (conn->interface, conn->channel_id)) { -DBG ("Failed to remove caif interface %s." , - conn->interface); - } - conn->cid =0 ; +CALLBACK_WITH_SUCCESS (cb, cbd->data);return ; error:
@@ -248,9 +241,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data) conn_compare_by_cid);if (!l) { -DBG ("Did not find data (device and channel id)" -"for connection with cid; %d" , - gcd->active_context); +DBG ("CAIF Device gone missing (cid:%d)" , gcd->active_context); goto error; }
@@ -291,20 +282,9 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data) dns[1 ] = rsp.dns_server2; dns[2 ] =NULL ; -sprintf (conn->interface,"caif%u" , conn->device); - -if (!caif_if_create (conn->interface, conn->channel_id)) { -ofono_error ("Failed to create caif interface %s." , - conn->interface); -CALLBACK_WITH_SUCCESS (cb,NULL , FALSE, rsp.ip_address, +CALLBACK_WITH_SUCCESS (cb, conn->interface, TRUE, rsp.ip_address, rsp.subnet_mask, rsp.default_gateway, dns, cbd->data); - }else { -CALLBACK_WITH_SUCCESS (cb, conn->interface, - FALSE, rsp.ip_address, rsp.subnet_mask, - rsp.default_gateway, dns, cbd->data); - } -return ; error:
@@ -316,6 +296,7 @@ error:if (conn) conn->cid =0 ; + gcd->active_context =0 ;CALLBACK_WITH_FAILURE (cb,NULL ,0 ,NULL ,NULL ,NULL ,NULL , cbd->data); }
@@ -327,38 +308,43 @@ static void ste_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)struct gprs_context_data *gcd =ofono_gprs_context_get_data (gc);struct cb_data *ncbd =NULL ;char buf[128 ]; -struct conn_info *conn; +struct conn_info *conn =NULL ;GSList *l; + l =g_slist_find_custom (g_caif_devices, +GUINT_TO_POINTER (gcd->active_context), + conn_compare_by_cid); + +if (!l) { +DBG ("CAIF Device gone missing (cid:%d)" , gcd->active_context); + goto error; + } + + conn = l->data; +if (!ok) {struct ofono_error error; + conn->cid =0 ; gcd->active_context =0 ; +decode_at_error (&error,g_at_result_final_response (result));cb (&error,NULL ,0 ,NULL ,NULL ,NULL ,NULL , cbd->data);return ; } - ncbd =g_memdup (cbd,sizeof (struct cb_data )); - - l =g_slist_find_custom (g_caif_devices,GUINT_TO_POINTER (0 ), - conn_compare_by_cid); - -if (!l) { -DBG ("at_cgdcont_cb, no more available devices" ); - goto error; - } - - conn = l->data; - conn->cid = gcd->active_context;snprintf (buf,sizeof (buf),"AT*EPPSD=1,%u,%u" , conn->channel_id, conn->cid); + ncbd =g_memdup (cbd,sizeof (struct cb_data ));if (g_at_chat_send (gcd->chat, buf,NULL , ste_eppsd_up_cb, ncbd, g_free) >0 )return ; error: +if (conn) + conn->cid =0 ; +g_free (ncbd); gcd->active_context =0 ;
@@ -375,6 +361,8 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,struct cb_data *cbd =cb_data_new (cb, data);char buf[AUTH_BUF_LENGTH ];int len; +GSList *l; +struct conn_info *conn =NULL ;if (!cbd) goto error;
@@ -382,6 +370,18 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc, gcd->active_context = ctx->cid; cbd->user = gc; +/* Find free connection with cid zero */ + l =g_slist_find_custom (g_caif_devices,GUINT_TO_POINTER (0 ), + conn_compare_by_cid); + +if (!l) { +DBG ("No more available CAIF devices" ); + goto error; + } + + conn = l->data; + conn->cid = ctx->cid; + len =snprintf (buf,sizeof (buf),"AT+CGDCONT=%u,\"IP\"" , ctx->cid);if (ctx->apn)
@@ -405,6 +405,9 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,return ; error: +if (conn) + conn->cid =0 ; + gcd->active_context =0 ;g_free (cbd);
@@ -431,8 +434,8 @@ static void ste_gprs_deactivate_primary(struct ofono_gprs_context *gc, conn_compare_by_cid);if (!l) { -DBG ("at_gprs_deactivate_primary, did not find" -"data (channel id) for connection with cid; %d" , id); +DBG ("did not find data (channel id) " +"for connection with cid; %d" , id); goto error; }
@@ -524,9 +527,11 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,ofono_gprs_context_set_data (gc, gcd);for (i =0 ; i <MAX_CAIF_DEVICES ; i++) { - ci =conn_info_create (i, i+1 ); -if (ci) - g_caif_devices =g_slist_append (g_caif_devices, ci); + ci =conn_info_create (i+1 ); +if (!ci) +return -ENOMEM ; +caif_if_create (ci); + g_caif_devices =g_slist_append (g_caif_devices, ci); }return 0 ;
@@ -536,6 +541,7 @@ static void ste_gprs_context_remove(struct ofono_gprs_context *gc) {struct gprs_context_data *gcd =ofono_gprs_context_get_data (gc); +g_slist_foreach (g_caif_devices, (GFunc ) caif_if_remove,NULL );g_slist_foreach (g_caif_devices, (GFunc ) g_free,NULL );g_slist_free (g_caif_devices); g_caif_devices =NULL ;
Signature · 1 line
1.6.3.3
Re: [PATCH v5b 2/3] stemodem: Create network interfaces statically
Hi Sjur,
drivers/stemodem/gprs-context.c static void ste_eppsd_down_cb(gboolean ok, GAtResult *result, · line 193
Quoted 8 lines
static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
drivers/stemodem/gprs-context.c +1
-1@@ -191,7 +189,7 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,ofono_gprs_context_cb_t cb = cbd->cb;struct ofono_gprs_context *gc = cbd->user;struct gprs_context_data *gcd =ofono_gprs_context_get_data (gc); -struct conn_info *conn; +struct conn_info *conn =NULL ;GSList *l;
why do you need the conn = NULL assignment here? I haven't even looked into your code flow, but I prefer clearly to not have these.
Regards
Marcel
Re: [PATCH v5b 2/3] stemodem: Create network interfaces statically
Hi Marcel.
Quoted 6 lines
Quoted 3 lines
- struct
conn_info*conn;
+ structconn_info*conn = NULL;
GSList *l;why do you need the conn = NULL assignment here? I haven't even looked into your code flow, but I prefer clearly to not have these.
I have removed this and tried to remove some of the other initialisations at declaration as well.
New patch set on the remaining gprs-context changes is on it's way :-)
Regards,
Sjur
PATCH 3/3 stemodem: Use RTNL to create interfaces
In reply to Sjur BrændelandFrom: Sjur Brændeland <[email protected]>
Use rtnl_caif_* functions to create network interfaces. The conn_info struct is also re-structured in order to hold the relevant information for network interfaces.
--- drivers/stemodem/gprs-context.c | 76 ++++++++++++++++++++++++++++++++++----- 1 files changed, 67 insertions(+), 9 deletions(-)
drivers/stemodem/gprs-context.c +67 -9
@@ -47,6 +47,7 @@#include "stemodem.h" #include "caif_socket.h" #include "if_caif.h" +#include "caif_rtnl.h" #define MAX_CAIF_DEVICES 4#define MAX_DNS 2
@@ -66,10 +67,18 @@ struct gprs_context_data { };struct conn_info { +/* +* cid is allocated in oFono Core and is identifying +* the Account. cid = 0 indicates that it is currently unused. +*/ unsigned int cid; -unsigned int device; +/* Id used by CAIF and EPPSD to identify the CAIF channel*/ unsigned int channel_id; -char interface[10 ]; +/* Linux Interface Id */ +unsigned int ifindex; +/* Linux Interface name */ +char interface[IF_NAMESIZE ]; +gboolean created; };struct eppsd_response {
@@ -77,7 +86,6 @@ struct eppsd_response {char ip_address[IP_ADDR_LEN ];char subnet_mask[IP_ADDR_LEN ];char mtu[IP_ADDR_LEN ]; -char default_gateway[IP_ADDR_LEN ];char dns_server1[IP_ADDR_LEN ];char dns_server2[IP_ADDR_LEN ];char p_cscf_server[IP_ADDR_LEN ];
@@ -97,8 +105,6 @@ static void start_element_handler(GMarkupParseContext *context, rsp->current = rsp->subnet_mask; else if (!strcmp(element_name, "mtu")) rsp->current = rsp->mtu; - else if (!strcmp(element_name, "default_gateway")) - rsp->current = rsp->default_gateway; else if (!strcmp(element_name, "dns_server") && rsp->dns_server1[0] == '\0') rsp->current = rsp->dns_server1;
@@ -124,7 +130,7 @@ static void text_handler(GMarkupParseContext *context,if (rsp->current) {strncpy (rsp->current, text,IP_ADDR_LEN ); - rsp->current[IP_ADDR_LEN ] =0 ; + rsp->current[IP_ADDR_LEN ] ='\0' ; } }
@@ -167,12 +173,38 @@ static struct conn_info *conn_info_create(unsigned int channel_id)return connection; } +static void rtnl_callback (int ifindex,char *ifname,void *user_data) +{ +struct conn_info *conn = user_data; + +if (ifindex <0 ) { + conn->created = FALSE; +ofono_error ("Failed to create caif interface %s" , + conn->interface); +return ; + } + +strncpy (conn->interface, ifname,sizeof (conn->interface)); + conn->ifindex = ifindex; + conn->created = TRUE; +} +/* * Creates a new IP interface for CAIF. */ static gboolean caif_if_create (struct conn_info *conn) { -return FALSE; +int err; + + err =caif_rtnl_create_interface (IFLA_CAIF_IPV4_CONNID , + conn->channel_id, FALSE, + rtnl_callback, conn); +if (err <0 ) { +DBG ("Failed to create IP interface for CAIF" ); +return FALSE; + } + +return TRUE; }/*
@@ -180,6 +212,18 @@ static gboolean caif_if_create(struct conn_info *conn)*/ static void caif_if_remove (struct conn_info *conn) { +if (!conn->created) +return ; + +if (caif_rtnl_delete_interface (conn->ifindex) <0 ) { +ofono_error ("Failed to delete caif interface %s" , + conn->interface); +return ; + } + +DBG ("removed CAIF interface ch:%d ifname:%s ifindex:%d\n" , + conn->channel_id, conn->interface, conn->ifindex); +return ; } static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
@@ -283,7 +327,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data) dns[2 ] =NULL ;CALLBACK_WITH_SUCCESS (cb, conn->interface, TRUE, rsp.ip_address, - rsp.subnet_mask, rsp.default_gateway, + rsp.subnet_mask,NULL , dns, cbd->data);return ;
@@ -380,6 +424,11 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc, } conn = l->data; +if (!conn->created) { +DBG ("CAIF interface not created (rtnl error?)" ); + goto error; + } + conn->cid = ctx->cid; len =snprintf (buf,sizeof (buf),"AT+CGDCONT=%u,\"IP\"" , ctx->cid);
@@ -460,6 +509,7 @@ static void ste_cgact_read_cb(gboolean ok, GAtResult *result,struct gprs_context_data *gcd =ofono_gprs_context_get_data (gc);gint cid, state;GAtResultIter iter; +GSList *l;if (!ok)return ;
@@ -483,6 +533,13 @@ static void ste_cgact_read_cb(gboolean ok, GAtResult *result,ofono_gprs_context_deactivated (gc, gcd->active_context); gcd->active_context =0 ; + l =g_slist_find_custom (g_caif_devices,GUINT_TO_POINTER (cid), + conn_compare_by_cid); +if (l) { +struct conn_info *conn = l->data; + conn->cid =0 ; + } +break ; } }
@@ -540,7 +597,6 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,static void ste_gprs_context_remove (struct ofono_gprs_context *gc) {struct gprs_context_data *gcd =ofono_gprs_context_get_data (gc); -g_slist_foreach (g_caif_devices, (GFunc ) caif_if_remove,NULL );g_slist_foreach (g_caif_devices, (GFunc ) g_free,NULL );g_slist_free (g_caif_devices);
@@ -562,10 +618,12 @@ static struct ofono_gprs_context_driver driver = {void ste_gprs_context_init () { +caif_rtnl_init ();ofono_gprs_context_driver_register (&driver); }void ste_gprs_context_exit () { +caif_rtnl_exit ();ofono_gprs_context_driver_unregister (&driver); }
Signature · 1 line
1.6.3.3