[PATCH 2/4][RFC] gatchat: Allow cancelling a running command.

[PATCH 2/4][RFC] gatchat: Allow cancelling a running command.

[Andrzej Zaborowski]

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

Users need to be extra careful using the cancel functions because
there's a potential race condition where an OK or ERROR for the command
being cancelled arrives just the same moment the next command in the
queue is being submitted, and is treated as a response to that command.

Ideally we would have some kind of "barrier" command that could be
inserted into the queue and whose response would be different than
OK or ERROR.
---
 gatchat/gatchat.c |   32 +++++++++++++++-----------------
 1 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/gatchat/gatchat.c b/gatchat/gatchat.c
index f94605f..4552767 100644
--- a/gatchat/gatchat.c
+++ b/gatchat/gatchat.c
@@ -1026,6 +1026,7 @@ guint g_at_chat_send_pdu_listing(GAtChat *chat, const char *cmd,
 gboolean g_at_chat_cancel(GAtChat *chat, guint id)
 {
 	GList *l;
+	gboolean head;
 
 	if (chat == NULL || chat->command_queue == NULL)
 		return FALSE;
@@ -1040,18 +1041,20 @@ gboolean g_at_chat_cancel(GAtChat *chat, guint id)
 	if (!l)
 		return FALSE;
 
-	if (l == g_queue_peek_head(chat->command_queue) &&
-			chat->cmd_bytes_written > 0) {
-		struct at_command *c = l->data;
+	head = l->data == g_queue_peek_head(chat->command_queue);
 
-		/* We can't actually remove it since it is most likely
-		 * already in progress, just null out the callback
-		 * so it won't be called
-		 */
-		c->callback = NULL;
-	} else {
-		at_command_destroy(l->data);
-		g_queue_remove(chat->command_queue, l->data);
+	at_command_destroy(l->data);
+	g_queue_remove(chat->command_queue, l->data);
+
+	if (head) {
+		chat->cmd_bytes_written = 0;
+
+		g_slist_foreach(chat->response_lines, (GFunc)g_free, NULL);
+		g_slist_free(chat->response_lines);
+		chat->response_lines = NULL;
+
+		if (g_queue_get_length(chat->command_queue) > 0)
+			chat_wakeup_writer(chat);
 	}
 
 	return TRUE;
@@ -1071,15 +1074,10 @@ gboolean g_at_chat_cancel_all(GAtChat *chat)
 			continue;
 		}
 
-		if (n == 0 && chat->cmd_bytes_written > 0) {
-			c->callback = NULL;
-			n += 1;
-			continue;
-		}
-
 		at_command_destroy(c);
 		g_queue_remove(chat->command_queue, c);
 	}
+	chat->cmd_bytes_written = 0;
 
 	return TRUE;
 }
-- 
1.6.1

Re: [PATCH 2/4][RFC] gatchat: Allow cancelling a running command.

[Denis Kenzior]

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

Hi Andrew,

> Users need to be extra careful using the cancel functions because
> there's a potential race condition where an OK or ERROR for the command
> being cancelled arrives just the same moment the next command in the
> queue is being submitted, and is treated as a response to that command.

This is why this is just a really bad idea.  The only one I think we should 
cancel (and most vendors support reliably) is an ATD...

Are you sure your CSIM STATUS poll would actually time out or would the modem 
return a +CME/+CMS error?  In the end the modem does this polling itself...

> @@ -1040,18 +1041,20 @@ gboolean g_at_chat_cancel(GAtChat *chat, guint id)
>  	if (!l)
>  		return FALSE;
> 
> -	if (l == g_queue_peek_head(chat->command_queue) &&
> -			chat->cmd_bytes_written > 0) {
> -		struct at_command *c = l->data;
> +	head = l->data == g_queue_peek_head(chat->command_queue);
> 
> -		/* We can't actually remove it since it is most likely
> -		 * already in progress, just null out the callback
> -		 * so it won't be called
> -		 */
> -		c->callback = NULL;
> -	} else {
> -		at_command_destroy(l->data);
> -		g_queue_remove(chat->command_queue, l->data);
> +	at_command_destroy(l->data);
> +	g_queue_remove(chat->command_queue, l->data);
> +
> +	if (head) {
> +		chat->cmd_bytes_written = 0;
> +
> +		g_slist_foreach(chat->response_lines, (GFunc)g_free, NULL);
> +		g_slist_free(chat->response_lines);
> +		chat->response_lines = NULL;
> +
> +		if (g_queue_get_length(chat->command_queue) > 0)
> +			chat_wakeup_writer(chat);

Quoting V.250 5.6.1:
"Some action commands that require time to execute may be aborted while in 
progress; these are explicitly noted in the description of the command. 
Aborting of commands is accomplished by the transmission from the DTE to the 
DCE of any character. A single character shall be sufficient to abort the 
command in progress; however, characters transmitted during the first 125 
milliseconds after transmission of the termination character shall be ignored 
(to allow for the DTE to append additional control characters such as line 
feed after the command line termination character)."

Sounds like your device's AT parser is 'not quite' compliant ;)

Regards,
-Denis

Re: [PATCH 2/4][RFC] gatchat: Allow cancelling a running command.

[Andrzej Zaborowski]

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

Hi Denis,

On 30 April 2010 04:53, Denis Kenzior <denkenz@gmail.com> wrote:
>> Users need to be extra careful using the cancel functions because
>> there's a potential race condition where an OK or ERROR for the command
>> being cancelled arrives just the same moment the next command in the
>> queue is being submitted, and is treated as a response to that command.
>
> This is why this is just a really bad idea.  The only one I think we should
> cancel (and most vendors support reliably) is an ATD...

You're right, the only modem it seems to work on is the Calypso now
that I've tried a bunch of different ones.

Let's skip this patch.

>
> Are you sure your CSIM STATUS poll would actually time out or would the modem
> return a +CME/+CMS error?  In the end the modem does this polling itself...

In practice they'll return errors, though according to the specs, we
should be assuming the card is not there if we get no response for N
seconds.

On the Ericsson F3607 I found it'll actually fake a response with
status word 148,4 when card is not inserted, and doesn't do polling,
i.e. insertion can only be detected if you re-connected the modem, and
extraction only when you issued a command and the modem couldn't reach
the card to perform the command.

Regards