[PATCH] mbmmodem: don't let chat open after fd is sent

[PATCH] mbmmodem: don't let chat open after fd is sent

[Lucas De Marchi]

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

Instead of using a GAtChat, just use a GIOChannel and close it as soon
as its fd is sent to core.

Moreover, when oFono exits it's just a matter of checking if there's an
idle callback scheduled to control if we need to close the GIOChannel.
---

Hi Denis,

Following patch implements what the previous one were trying to accomplish, but
without the problem of closing the chat before the command was sent to tty. As
we talked on IRC, now I'm using the GIOChannel directly, without bothering with
creating the chat. Even if the command to send to this channel is very short, I
preferred to use async calls. Hence the use of an idle callback.


regards,
Lucas De Marchi


 drivers/mbmmodem/location-reporting.c |  105 +++++++++++++++++++--------------
 1 files changed, 60 insertions(+), 45 deletions(-)

diff --git a/drivers/mbmmodem/location-reporting.c b/drivers/mbmmodem/location-reporting.c
index 941fac4..30571fe 100644
--- a/drivers/mbmmodem/location-reporting.c
+++ b/drivers/mbmmodem/location-reporting.c
@@ -48,7 +48,14 @@ static const char *e2gpsctl_prefix[] = { "*E2GPSCTL:", NULL };
 
 struct gps_data {
 	GAtChat *chat;
-	GAtChat *data_chat;
+
+	struct {
+		GIOChannel *channel;
+		guint idle_source;
+		gsize written;
+		ofono_location_reporting_enable_cb_t cb;
+		void *cb_data;
+	} stream;
 };
 
 static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
@@ -57,7 +64,6 @@ static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
 	struct cb_data *cbd = user_data;
 	struct ofono_location_reporting *lr = cbd->user;
 	ofono_location_reporting_disable_cb_t cb = cbd->cb;
-	struct gps_data *gd = ofono_location_reporting_get_data(lr);
 
 	DBG("lr=%p, ok=%d", lr, ok);
 
@@ -70,8 +76,6 @@ static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
 		return;
 	}
 
-	g_at_chat_unref(gd->data_chat);
-
 	CALLBACK_WITH_SUCCESS(cb, cbd->data);
 }
 
@@ -94,69 +98,81 @@ static void mbm_location_reporting_disable(struct ofono_location_reporting *lr,
 	g_free(cbd);
 }
 
-static int mbm_create_data_chat(struct ofono_location_reporting *lr)
+static gboolean enable_data_stream(gpointer data)
 {
-	struct gps_data *gd = ofono_location_reporting_get_data(lr);
-	struct ofono_modem *modem;
-	const char *gps_dev;
-	GAtSyntax *syntax;
-	GIOChannel *channel;
+	static const char *buf = "AT*E2GPSNPD\r\n";
+	gsize len = strlen(buf);
+	struct gps_data *gd = data;
+	GIOStatus status;
+	gsize written;
 	int fd;
 
-	modem = ofono_location_reporting_get_modem(lr);
-	gps_dev = ofono_modem_get_string(modem, "GPSDevice");
+	status = g_io_channel_write_chars(gd->stream.channel,
+						buf + gd->stream.written,
+						len - gd->stream.written,
+						&written, NULL);
+	if (status == G_IO_STATUS_AGAIN)
+		return TRUE;
 
-	channel = g_at_tty_open(gps_dev, NULL);
-	if (channel == NULL)
-		return -1;
+	if (status != G_IO_STATUS_NORMAL) {
+		CALLBACK_WITH_FAILURE(gd->stream.cb, -1, gd->stream.cb_data);
 
-	syntax = g_at_syntax_new_gsm_permissive();
-	gd->data_chat = g_at_chat_new(channel, syntax);
-	fd = g_io_channel_unix_get_fd(channel);
+		return FALSE;
+	}
+
+	gd->stream.written += written;
 
-	g_at_syntax_unref(syntax);
-	g_io_channel_unref(channel);
+	if (gd->stream.written != len)
+		return TRUE;
 
-	if (gd->data_chat == NULL)
-		return -1;
+	fd = g_io_channel_unix_get_fd(gd->stream.channel);
+	CALLBACK_WITH_SUCCESS(gd->stream.cb, fd, gd->stream.cb_data);
 
-	return fd;
+	return FALSE;
+}
+
+static void destroy_data_stream(gpointer data)
+{
+	struct gps_data *gd = data;
+
+	g_io_channel_unref(gd->stream.channel);
+	gd->stream.idle_source = 0;
+	gd->stream.written = 0;
 }
 
 static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
 					gpointer user_data)
 {
-	struct cb_data *cbd = user_data;
-	ofono_location_reporting_enable_cb_t cb = cbd->cb;
-	struct ofono_location_reporting *lr = cbd->user;
+	struct ofono_location_reporting *lr = user_data;
 	struct gps_data *gd = ofono_location_reporting_get_data(lr);
+	struct ofono_modem *modem;
+	const char *gps_dev;
 	struct ofono_error error;
-	int fd;
 
 	DBG("lr=%p ok=%d", lr, ok);
 
 	decode_at_error(&error, g_at_result_final_response(result));
 
 	if (!ok) {
-		cb(&error, -1, cbd->data);
+		gd->stream.cb(&error, -1, gd->stream.cb_data);
 
 		return;
 	}
 
-	fd = mbm_create_data_chat(lr);
-
-	if (fd < 0)
-		goto out;
+	modem = ofono_location_reporting_get_modem(lr);
+	gps_dev = ofono_modem_get_string(modem, "GPSDevice");
+	gd->stream.channel = g_at_tty_open(gps_dev, NULL);
 
-	if (g_at_chat_send(gd->data_chat, "AT*E2GPSNPD", NULL, NULL, NULL,
-								NULL) > 0) {
-		cb(&error, fd, cbd->data);
+	if (gd->stream.channel == NULL) {
+		ofono_error("Could select data GPS channel");
+		CALLBACK_WITH_FAILURE(gd->stream.cb, -1, gd->stream.cb_data);
 
 		return;
 	}
 
-out:
-	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+	gd->stream.idle_source = g_idle_add_full(G_PRIORITY_DEFAULT_IDLE,
+							enable_data_stream, gd,
+							destroy_data_stream);
 }
 
 static void mbm_location_reporting_enable(struct ofono_location_reporting *lr,
@@ -164,22 +180,17 @@ static void mbm_location_reporting_enable(struct ofono_location_reporting *lr,
 					void *data)
 {
 	struct gps_data *gd = ofono_location_reporting_get_data(lr);
-	struct cb_data *cbd = cb_data_new(cb, data);
 
 	DBG("lr=%p", lr);
 
-	if (cbd == NULL)
-		goto out;
-
-	cbd->user = lr;
+	gd->stream.cb_data = data;
+	gd->stream.cb = cb;
 
 	if (g_at_chat_send(gd->chat, "AT*E2GPSCTL=1,5,1", none_prefix,
-				mbm_e2gpsctl_enable_cb, cbd, g_free) > 0)
+				mbm_e2gpsctl_enable_cb, lr, NULL) > 0)
 		return;
 
-out:
 	CALLBACK_WITH_FAILURE(cb, -1, data);
-	g_free(cbd);
 }
 
 static void mbm_location_reporting_support_cb(gboolean ok, GAtResult *result,
@@ -224,6 +235,10 @@ static void mbm_location_reporting_remove(struct ofono_location_reporting *lr)
 	ofono_location_reporting_set_data(lr, NULL);
 
 	g_at_chat_unref(gd->chat);
+
+	if (gd->stream.idle_source)
+		g_source_remove(gd->stream.idle_source);
+
 	g_free(gd);
 }
 
-- 
1.7.4.1

Re: [PATCH] mbmmodem: don't let chat open after fd is sent

[Marcel Holtmann]

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

Hi Lucas,

>  struct gps_data {
>  	GAtChat *chat;
> -	GAtChat *data_chat;
> +
> +	struct {
> +		GIOChannel *channel;
> +		guint idle_source;
> +		gsize written;
> +		ofono_location_reporting_enable_cb_t cb;
> +		void *cb_data;
> +	} stream;
>  };

why do you bother with the struct in struct here?

Regards

Marcel

Re: [PATCH] mbmmodem: don't let chat open after fd is sent

[Lucas De Marchi]

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

On Thu, Mar 3, 2011 at 2:21 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Lucas,
>
>>  struct gps_data {
>>       GAtChat *chat;
>> -     GAtChat *data_chat;
>> +
>> +     struct {
>> +             GIOChannel *channel;
>> +             guint idle_source;
>> +             gsize written;
>> +             ofono_location_reporting_enable_cb_t cb;
>> +             void *cb_data;
>> +     } stream;
>>  };
>
> why do you bother with the struct in struct here?

Just to group these related fields.


regards,
Lucas De Marchi

Re: [PATCH] mbmmodem: don't let chat open after fd is sent

[Marcel Holtmann]

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

Hi Lucas,

> >>  struct gps_data {
> >>       GAtChat *chat;
> >> -     GAtChat *data_chat;
> >> +
> >> +     struct {
> >> +             GIOChannel *channel;
> >> +             guint idle_source;
> >> +             gsize written;
> >> +             ofono_location_reporting_enable_cb_t cb;
> >> +             void *cb_data;
> >> +     } stream;
> >>  };
> >
> > why do you bother with the struct in struct here?
> 
> Just to group these related fields.

I let Denis decide to take this or not. My preference is to not do this
unless it has some clear benefit, but that is just me ;)

Regards

Marcel

[PATCH] mbmmodem: don't let chat open after fd is sent

[Lucas De Marchi]

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

Instead of using a GAtChat, just use a GIOChannel and close it as soon
as its fd is sent to core.
---
 drivers/mbmmodem/location-reporting.c |   32 +++++++++++++++-----------------
 1 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/mbmmodem/location-reporting.c b/drivers/mbmmodem/location-reporting.c
index 941fac4..b76a5c2 100644
--- a/drivers/mbmmodem/location-reporting.c
+++ b/drivers/mbmmodem/location-reporting.c
@@ -94,13 +94,13 @@ static void mbm_location_reporting_disable(struct ofono_location_reporting *lr,
 	g_free(cbd);
 }
 
-static int mbm_create_data_chat(struct ofono_location_reporting *lr)
+static int enable_data_stream(struct ofono_location_reporting *lr)
 {
-	struct gps_data *gd = ofono_location_reporting_get_data(lr);
 	struct ofono_modem *modem;
 	const char *gps_dev;
-	GAtSyntax *syntax;
 	GIOChannel *channel;
+	GIOStatus status;
+	gsize written;
 	int fd;
 
 	modem = ofono_location_reporting_get_modem(lr);
@@ -110,15 +110,18 @@ static int mbm_create_data_chat(struct ofono_location_reporting *lr)
 	if (channel == NULL)
 		return -1;
 
-	syntax = g_at_syntax_new_gsm_permissive();
-	gd->data_chat = g_at_chat_new(channel, syntax);
 	fd = g_io_channel_unix_get_fd(channel);
+	status = g_io_channel_write_chars(channel, "AT*E2GPSNPD\r\n", -1,
+								&written, NULL);
 
-	g_at_syntax_unref(syntax);
+	g_io_channel_set_close_on_unref(channel, FALSE);
 	g_io_channel_unref(channel);
 
-	if (gd->data_chat == NULL)
+	if (status != G_IO_STATUS_NORMAL || written != 13) {
+		close(fd);
+
 		return -1;
+	}
 
 	return fd;
 }
@@ -129,7 +132,6 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
 	struct cb_data *cbd = user_data;
 	ofono_location_reporting_enable_cb_t cb = cbd->cb;
 	struct ofono_location_reporting *lr = cbd->user;
-	struct gps_data *gd = ofono_location_reporting_get_data(lr);
 	struct ofono_error error;
 	int fd;
 
@@ -143,20 +145,16 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
 		return;
 	}
 
-	fd = mbm_create_data_chat(lr);
+	fd = enable_data_stream(lr);
 
-	if (fd < 0)
-		goto out;
-
-	if (g_at_chat_send(gd->data_chat, "AT*E2GPSNPD", NULL, NULL, NULL,
-								NULL) > 0) {
-		cb(&error, fd, cbd->data);
+	if (fd < 0) {
+		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
 
 		return;
 	}
 
-out:
-	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+	cb(&error, fd, cbd->data);
+	close(fd);
 }
 
 static void mbm_location_reporting_enable(struct ofono_location_reporting *lr,
-- 
1.7.4.1

Re: [PATCH] mbmmodem: don't let chat open after fd is sent

[Denis Kenzior]

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

Hi Lucas,

On 03/03/2011 04:23 PM, Lucas De Marchi wrote:
> Instead of using a GAtChat, just use a GIOChannel and close it as soon
> as its fd is sent to core.
> ---
>  drivers/mbmmodem/location-reporting.c |   32 +++++++++++++++-----------------
>  1 files changed, 15 insertions(+), 17 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis