Re: [RFC 3/3] STE-plugin: Adding STE plugin

[Marcel Holtmann <marcel@holtmann.org>]

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

Hi Sjur,

> Added implementation for STE modem; STE modem driver, and STE specific
> drivers for gprs, network registration and voice call. 
> 
> This patch uses CAIF sockets. CAIF patch for net-next-2.6 will be
> contributed on netdev(a)vger.kernel.org soon.

can you please split these into smaller patches so they are easier to
review. I am thinking of something the like this; one per network
registration, one per voice call, one per GPRS.

You are making some core changes and we need to have a close look at
them and what the potential impact would be.

This is not a complete review, but I making some comments on obvious
things.

> index 0000000..1d5d8db
> --- /dev/null
> +++ b/drivers/stemodem/gprs-context.c
> @@ -0,0 +1,612 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
> + *  Copyright (C) 2010 ST-Ericsson AB.
> + *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.

As mentioned before, we track authorship via GIT. So ensure that the GIT
commits are properly done.

> +#include <linux/types.h>
> +#include <net/if.h>
> +#include <sys/ioctl.h>
> +#include <arpa/inet.h>
> +#include <linux/caif/caif_socket.h>
> +#include <linux/caif/if_caif.h>

This is dangerous until the CAIF subsystem is actually merged and
present everywhere. Please consider adding an option to enable stemodem
driver (like we do for atmodem and isimodem).

It might make sense to have a local copy of the required structure and
constants to allow an easier complication. Of course this depends on
having CAIF at least in net-next-2.6 tree.

> +/* TODO: should parse_xml function to be moved to e.g. atutil? */

First question. Why not use the XML parse that comes with GLib.

> +static char *parse_xml(char * xml, char* tag)
> +{

Please use consistent coding style. It is "char *xml". And it is always
like this. No extra space after * or char*.

> +    char *begin;
> +    char *end;
> +    int len;
> +    char *res = NULL;
> +    char *start = (char *)g_malloc(strlen(tag)+3);
> +    char *stop = (char *)g_malloc(strlen(tag)+4);

No casting of malloc function. It is not needed. And extra spaces
between operation. Meaning malloc(strlen(tag) + 3).

This applies to all code.

> +	if (create) {
> +		if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> +			ofono_debug("Failed to create IP interface for CAIF");
> +			goto error;
> +		}
> +
> +		s = socket(PF_INET, SOCK_DGRAM, 0);
> +		if (s < 0) {
> +			ofono_debug("Failed to create socket.");
> +			goto error;
> +		}
> +
> +		/* Set IP address */
> +		memset(&sin, 0, sizeof(struct sockaddr));
> +		sin.sin_family = AF_INET;
> +
> +		if (inet_pton(AF_INET, ip, &sin.sin_addr) <= 0) {
> +			ofono_debug("inet_pton failed, will not be"
> +				"able to set the IP address");
> +			goto error;
> +		}
> +		memcpy(&ifr.ifr_addr, &sin, sizeof(struct sockaddr));
> +
> +		if (ioctl(s, SIOCSIFADDR, &ifr) < 0) {
> +			ofono_debug("Failed to set IP address for"
> +			" interface: %s", ifr.ifr_name);
> +			goto error;
> +		}

oFono is never setting the IP address, netmask or any other
configuration option. The only thing that oFono does is bringing the
interface up. Systems like ConnMan do the IP configuration.

Please see the comments in the documentation on how we expose IP
interfaces. Check with ConnMan and how we configure them.

> +		if (ioctl(s, SIOCSIFMTU, &ifr) < 0)
> +			ofono_debug("Failed to set MTU for interface: %s",
> +				ifr.ifr_name);
> +	} else {
> +		if (ioctl(s, SIOCGIFINDEX, &ifr) == 0) {
> +			if (ioctl(s, SIOCCAIFNETREMOVE, &ifr) < 0) {
> +				ofono_debug("Failed to remove IP interface"
> +					"for CAIF");
> +				goto error;
> +			}
> +		} else {
> +			ofono_debug("Did not find interface (%s) to remove",
> +					interface);
> +			goto error;
> +		}
> +	}

Having create and remove in the same function seems hard to read. Why
not have one for creating the interface and one for removing it. From
what I see so far, it is not much more code. And makes it a lot easier
to read and understand for other people.

> +	g_at_result_iter_init(&iter, result);
> +	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));
> +
> +		if (strstr(res_string, "ip_address")) {
> +			ip = g_strdup(parse_xml(res_string,
> +						"ip_address"));
> +		} else if ((strstr(res_string, "subnet_mask"))) {
> +			netmask = g_strdup(parse_xml(res_string,
> +						"subnet_mask"));
> +		} else if ((strstr(res_string, "mtu"))) {
> +			mtu = g_strdup(parse_xml(res_string,
> +						"mtu"));
> +		} else if ((strstr(res_string, "default_gateway"))) {
> +			gateway = g_strdup(parse_xml(res_string,
> +						"default_gateway"));
> +		} else if ((strstr(res_string, "dns_server"))) {
> +			str = g_strdup(parse_xml(res_string,
> +						"dns_server"));
> +
> +			if (numdns < MAX_DNS)
> +				dns[numdns++] = str;
> +		}

I would prefer if you do the parsing in one function that goes through
the XML ones. Just give it pointers to the fields you wanna have it
extract or a special combined structure. This looks highly inefficient
to me.

> +	conn->interface = g_malloc(10);
> +	if (!conn->interface)
> +		goto error;

Please double check with the GLib documentation on memory allocation.
The g_malloc failure would result in halt of the program. What you want
here is g_try_malloc.

Also for 10 bytes, please don't bother with an allocation. Just include
a char array in the conn structure.

> +
> +	sprintf(conn->interface, "caif%u", conn->device);
> +
> +	if (!caif_if_create_remove(conn->interface, ip,
> +			mtu, netmask, conn->channel_id, TRUE)) {
> +		ofono_error("Failed to create caif interface %s.",
> +			conn->interface);
> +		conn->interface = NULL;
> +	}

Yeah, I really prefer caif_if_create() and caif_if_remove(). This is not
really intuitive at all.

> +	/* Need to change to gsm_permissive syntax in order to
> +	 * parse the response from EPPSD (xml) */
> +	g_at_chat_set_syntax(gcd->chat, g_at_syntax_new_gsm_permissive());

Is this an issue with your modem firmware or an issue in the v1 parser.
If it is the modem firmware, then just use the permissive parser all the
time and switch to E0.

> +static void cgev_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_gprs_context *gc = user_data;
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	GAtResultIter iter;
> +	const char *event;
> +
> +	dump_response("cgev_notify", TRUE, result);
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CGEV:"))
> +		return;
> +
> +	if (!g_at_result_iter_next_unquoted_string(&iter, &event))
> +		return;
> +
> +	if (g_str_has_prefix(event, "NW REACT ") ||
> +			g_str_has_prefix(event, "NW DEACT ") ||
> +			g_str_has_prefix(event, "ME DEACT ")) {
> +		/* Ask what primary contexts are active now */
> +
> +		g_at_chat_send(gcd->chat, "AT+CGACT?", cgact_prefix,
> +				ste_cgact_read_cb, gc, NULL);
> +
> +		return;
> +	}
> +}

The return statement in the if clause is kinda pointless. Seems like
either your code tried to be more complex or you are missing something.

> +static int stemodem_init(void)
> +{
> +	/* Initialize voicecall driver */
> +	struct ofono_voicecall_driver *at_vcdrv;
> +	struct ofono_voicecall_driver *ste_vcdrv;
> +	struct ofono_netreg_driver *at_netdrv;
> +	struct ofono_netreg_driver *ste_netdrv;
> +
> +	ste_voicecall_init();
> +
> +	at_vcdrv = ofono_voicecall_driver_get("atmodem");
> +	ste_vcdrv = ofono_voicecall_driver_get("stemodem");
> +
> +	if (at_vcdrv && ste_vcdrv) {
> +		ste_vcdrv->remove = at_vcdrv->remove;
> +		ste_vcdrv->swap_without_accept = at_vcdrv->swap_without_accept;
> +		ste_vcdrv->send_tones = at_vcdrv->send_tones;
> +	} else {
> +		if (!ste_vcdrv)
> +			ofono_debug("Could not get ofono_voicecall_driver"
> +					"from stemodem");
> +		if (!at_vcdrv)
> +			ofono_debug("Could not get ofono_voicecall_driver"
> +					"from atmodem");
> +	}
> +
> +	/* Initialize netreg driver */
> +	ste_netreg_init();
> +
> +	at_netdrv = ofono_netreg_driver_get("atmodem");
> +	ste_netdrv = ofono_netreg_driver_get("stemodem");
> +
> +	if (at_netdrv && ste_netdrv) {
> +		ste_netdrv->remove = at_netdrv->remove;
> +		ste_netdrv->registration_status =
> +			at_netdrv->registration_status;
> +		ste_netdrv->current_operator = at_netdrv->current_operator;
> +		ste_netdrv->list_operators = at_netdrv->list_operators;
> +		ste_netdrv->register_auto = at_netdrv->register_auto;
> +		ste_netdrv->register_manual = at_netdrv->register_manual;

So far this, I really like to see a description on what's the difference
in the network registration and voice call atoms.

We do need to understand what is actually needed.

> +	/* Create a CAIF socket for AT Service */
> +	fd = socket(AF_CAIF, SOCK_SEQPACKET, CAIFPROTO_AT);
> +
> +	/* Connect to the AT Service at the modem */
> +	connect(fd, (struct sockaddr *) &addr, sizeof(addr));
> +	channel = g_io_channel_unix_new(fd);

You need to check the results of socket() and connect() calls.

Regards

Marcel