[PATCH 1/3] mbmmodem: close chat before sending fd

[PATCH 1/3] mbmmodem: close chat before sending fd

[Lucas De Marchi]

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

---
 drivers/mbmmodem/location-reporting.c |   44 ++++++++++++++++++---------------
 1 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/mbmmodem/location-reporting.c b/drivers/mbmmodem/location-reporting.c
index 941fac4..b671d71 100644
--- a/drivers/mbmmodem/location-reporting.c
+++ b/drivers/mbmmodem/location-reporting.c
@@ -48,7 +48,7 @@ static const char *e2gpsctl_prefix[] = { "*E2GPSCTL:", NULL };
 
 struct gps_data {
 	GAtChat *chat;
-	GAtChat *data_chat;
+	GIOChannel *data_channel;
 };
 
 static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
@@ -70,7 +70,7 @@ static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
 		return;
 	}
 
-	g_at_chat_unref(gd->data_chat);
+	g_io_channel_unref(gd->data_channel);
 
 	CALLBACK_WITH_SUCCESS(cb, cbd->data);
 }
@@ -94,33 +94,33 @@ 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 GAtChat *mbm_create_data_chat(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;
-	int fd;
+	GAtChat *chat;
 
 	modem = ofono_location_reporting_get_modem(lr);
 	gps_dev = ofono_modem_get_string(modem, "GPSDevice");
 
-	channel = g_at_tty_open(gps_dev, NULL);
-	if (channel == NULL)
-		return -1;
+	gd->data_channel = g_at_tty_open(gps_dev, NULL);
+	if (gd->data_channel == NULL)
+		return NULL;
 
 	syntax = g_at_syntax_new_gsm_permissive();
-	gd->data_chat = g_at_chat_new(channel, syntax);
-	fd = g_io_channel_unix_get_fd(channel);
+	chat = g_at_chat_new(gd->data_channel, syntax);
 
 	g_at_syntax_unref(syntax);
-	g_io_channel_unref(channel);
 
-	if (gd->data_chat == NULL)
-		return -1;
+	if (chat == NULL) {
+		g_io_channel_unref(gd->data_channel);
+
+		return NULL;
+	}
 
-	return fd;
+	return chat;
 }
 
 static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
@@ -131,7 +131,7 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
 	struct ofono_location_reporting *lr = cbd->user;
 	struct gps_data *gd = ofono_location_reporting_get_data(lr);
 	struct ofono_error error;
-	int fd;
+	GAtChat *chat;
 
 	DBG("lr=%p ok=%d", lr, ok);
 
@@ -143,18 +143,22 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
 		return;
 	}
 
-	fd = mbm_create_data_chat(lr);
-
-	if (fd < 0)
+	chat = mbm_create_data_chat(lr);
+	if (chat == NULL)
 		goto out;
 
-	if (g_at_chat_send(gd->data_chat, "AT*E2GPSNPD", NULL, NULL, NULL,
-								NULL) > 0) {
+	if (g_at_chat_send(chat, "AT*E2GPSNPD", NULL, NULL, NULL, NULL) > 0) {
+		int fd = g_io_channel_unix_get_fd(gd->data_channel);
+
+		g_at_chat_unref(chat);
 		cb(&error, fd, cbd->data);
 
 		return;
 	}
 
+	g_at_chat_unref(chat);
+	g_io_channel_unref(gd->data_channel);
+
 out:
 	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
 }
-- 
1.7.4.1

[PATCH 2/3] location-reporting: don't add client-exit watch too early

[Lucas De Marchi]

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

Wait until driver gives us a file descriptor to start watching for
client exit. This fixes a race when client exits before the driver
calls location_reporting_enable_cb().
---
 src/location-reporting.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/location-reporting.c b/src/location-reporting.c
index 6ab0914..f19ccf5 100644
--- a/src/location-reporting.c
+++ b/src/location-reporting.c
@@ -170,13 +170,12 @@ static void location_reporting_enable_cb(const struct ofono_error *error,
 							int fd,	void *data)
 {
 	struct ofono_location_reporting *lr = data;
+	DBusConnection *conn = ofono_dbus_get_connection();
 	DBusMessage *reply;
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		ofono_error("Enabling location-reporting failed");
 
-		client_remove(lr);
-
 		reply = __ofono_error_failed(lr->pending);
 		__ofono_dbus_pending_reply(&lr->pending, reply);
 
@@ -184,6 +183,9 @@ static void location_reporting_enable_cb(const struct ofono_error *error,
 	}
 
 	lr->enabled = TRUE;
+	lr->client_owner = g_strdup(dbus_message_get_sender(lr->pending));
+	lr->disconnect_watch = g_dbus_add_disconnect_watch(conn,
+				lr->client_owner, client_exited, lr, NULL);
 
 	reply = dbus_message_new_method_return(lr->pending);
 	dbus_message_append_args(reply, DBUS_TYPE_UNIX_FD, &fd,
@@ -198,7 +200,6 @@ static DBusMessage *location_reporting_request(DBusConnection *conn,
 						DBusMessage *msg, void *data)
 {
 	struct ofono_location_reporting *lr = data;
-	const char *caller = dbus_message_get_sender(msg);
 
 	if (lr->pending != NULL)
 		return __ofono_error_busy(msg);
@@ -206,9 +207,6 @@ static DBusMessage *location_reporting_request(DBusConnection *conn,
 	if (lr->enabled)
 		return __ofono_error_in_use(msg);
 
-	lr->client_owner = g_strdup(caller);
-	lr->disconnect_watch = g_dbus_add_disconnect_watch(conn,
-				lr->client_owner, client_exited, lr, NULL);
 	lr->pending = dbus_message_ref(msg);
 
 	lr->driver->enable(lr, location_reporting_enable_cb, lr);
-- 
1.7.4.1

Re: [PATCH 2/3] location-reporting: don't add client-exit watch too early

[Denis Kenzior]

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

Hi Lucas,

On 02/28/2011 07:43 AM, Lucas De Marchi wrote:
> Wait until driver gives us a file descriptor to start watching for
> client exit. This fixes a race when client exits before the driver
> calls location_reporting_enable_cb().
> ---
>  src/location-reporting.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

[PATCH 3/3] mbmmodem: ensure we close fd when ofono exits

[Lucas De Marchi]

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

---
 drivers/mbmmodem/location-reporting.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/mbmmodem/location-reporting.c b/drivers/mbmmodem/location-reporting.c
index b671d71..24ff4b8 100644
--- a/drivers/mbmmodem/location-reporting.c
+++ b/drivers/mbmmodem/location-reporting.c
@@ -71,6 +71,7 @@ static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
 	}
 
 	g_io_channel_unref(gd->data_channel);
+	gd->data_channel = NULL;
 
 	CALLBACK_WITH_SUCCESS(cb, cbd->data);
 }
@@ -116,6 +117,7 @@ static GAtChat *mbm_create_data_chat(struct ofono_location_reporting *lr)
 
 	if (chat == NULL) {
 		g_io_channel_unref(gd->data_channel);
+		gd->data_channel = NULL;
 
 		return NULL;
 	}
@@ -158,6 +160,7 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
 
 	g_at_chat_unref(chat);
 	g_io_channel_unref(gd->data_channel);
+	gd->data_channel = NULL;
 
 out:
 	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
@@ -225,6 +228,9 @@ static void mbm_location_reporting_remove(struct ofono_location_reporting *lr)
 {
 	struct gps_data *gd = ofono_location_reporting_get_data(lr);
 
+	if (gd->data_channel != NULL)
+		g_io_channel_unref(gd->data_channel);
+
 	ofono_location_reporting_set_data(lr, NULL);
 
 	g_at_chat_unref(gd->chat);
-- 
1.7.4.1

Re: [PATCH 1/3] mbmmodem: close chat before sending fd

[Denis Kenzior]

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

Hi Lucas,

On 02/28/2011 07:43 AM, Lucas De Marchi wrote:
> ---
>  drivers/mbmmodem/location-reporting.c |   44 ++++++++++++++++++---------------
>  1 files changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mbmmodem/location-reporting.c b/drivers/mbmmodem/location-reporting.c
> index 941fac4..b671d71 100644
> --- a/drivers/mbmmodem/location-reporting.c
> +++ b/drivers/mbmmodem/location-reporting.c
> @@ -48,7 +48,7 @@ static const char *e2gpsctl_prefix[] = { "*E2GPSCTL:", NULL };
>  
>  struct gps_data {
>  	GAtChat *chat;
> -	GAtChat *data_chat;
> +	GIOChannel *data_channel;
>  };
>  
>  static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
> @@ -70,7 +70,7 @@ static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
>  		return;
>  	}
>  
> -	g_at_chat_unref(gd->data_chat);
> +	g_io_channel_unref(gd->data_channel);
>  
>  	CALLBACK_WITH_SUCCESS(cb, cbd->data);
>  }
> @@ -94,33 +94,33 @@ 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 GAtChat *mbm_create_data_chat(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;
> -	int fd;
> +	GAtChat *chat;
>  
>  	modem = ofono_location_reporting_get_modem(lr);
>  	gps_dev = ofono_modem_get_string(modem, "GPSDevice");
>  
> -	channel = g_at_tty_open(gps_dev, NULL);
> -	if (channel == NULL)
> -		return -1;
> +	gd->data_channel = g_at_tty_open(gps_dev, NULL);
> +	if (gd->data_channel == NULL)
> +		return NULL;
>  
>  	syntax = g_at_syntax_new_gsm_permissive();
> -	gd->data_chat = g_at_chat_new(channel, syntax);
> -	fd = g_io_channel_unix_get_fd(channel);
> +	chat = g_at_chat_new(gd->data_channel, syntax);
>  
>  	g_at_syntax_unref(syntax);
> -	g_io_channel_unref(channel);
>  
> -	if (gd->data_chat == NULL)
> -		return -1;
> +	if (chat == NULL) {
> +		g_io_channel_unref(gd->data_channel);
> +
> +		return NULL;
> +	}
>  
> -	return fd;
> +	return chat;
>  }
>  
>  static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
> @@ -131,7 +131,7 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
>  	struct ofono_location_reporting *lr = cbd->user;
>  	struct gps_data *gd = ofono_location_reporting_get_data(lr);
>  	struct ofono_error error;
> -	int fd;
> +	GAtChat *chat;
>  
>  	DBG("lr=%p ok=%d", lr, ok);
>  
> @@ -143,18 +143,22 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
>  		return;
>  	}
>  
> -	fd = mbm_create_data_chat(lr);
> -
> -	if (fd < 0)
> +	chat = mbm_create_data_chat(lr);
> +	if (chat == NULL)
>  		goto out;
>  
> -	if (g_at_chat_send(gd->data_chat, "AT*E2GPSNPD", NULL, NULL, NULL,
> -								NULL) > 0) {
> +	if (g_at_chat_send(chat, "AT*E2GPSNPD", NULL, NULL, NULL, NULL) > 0) {

Are you sure this actually works as intended?  By default chat uses
non-blocking writes and waits for the write watcher to fire.  So just
because we succeed here does not mean anything has been written to the
channel.  It just means we queued something up and created a write watch.

> +		int fd = g_io_channel_unix_get_fd(gd->data_channel);
> +
> +		g_at_chat_unref(chat);

And here you destroy the write queue and the write watch.

>  		cb(&error, fd, cbd->data);
>  
>  		return;
>  	}
>  
> +	g_at_chat_unref(chat);
> +	g_io_channel_unref(gd->data_channel);
> +
>  out:
>  	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
>  }

Regards,
-Denis

[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