[PATCH v2] port-serial: rework response parsing
Aleksander Morgado
aleksander at aleksander.es
Fri Jan 22 07:30:34 PST 2016
Response parsing was being done in different places for AT and QCDM subclasses;
in the case of AT it was being done early, before returning the byte array in
the mm_serial_port_command_finish() response. In the case of QCDM, it was being
done after mm_serial_port_command_finish(), and that was forcing every caller to
cleanup the response buffer once the response was processed.
With the new logic in this patch, the response is always parsed (i.e. looked for
a valid response or an error detected) before mm_serial_port_command_finish()
returns, and actually this method now returns a totally different GByteArray,
not the internal response buffer GByteArray, so there's no longer any need for
the caller to explicitly clean it up. The one doing the cleanup is the parser
method itself in every case.
This change also allows us to return serial port responses in idle, but that's
not changed for now as there's no immediate need.
---
Hey,
Updated the patch to avoid breaking QCDM :)
---
src/mm-port-serial-at.c | 40 +++++++++-----
src/mm-port-serial-gps.c | 11 ++--
src/mm-port-serial-qcdm.c | 134 +++++++++++++++++++++++-----------------------
src/mm-port-serial.c | 118 +++++++++++++++++++++-------------------
src/mm-port-serial.h | 34 ++++++++----
5 files changed, 189 insertions(+), 148 deletions(-)
diff --git a/src/mm-port-serial-at.c b/src/mm-port-serial-at.c
index 553596e..9cab855 100644
--- a/src/mm-port-serial-at.c
+++ b/src/mm-port-serial-at.c
@@ -116,14 +116,16 @@ mm_port_serial_at_remove_echo (GByteArray *response)
}
}
-static gboolean
+static MMPortSerialResponseType
parse_response (MMPortSerial *port,
GByteArray *response,
+ GByteArray **parsed_response,
GError **error)
{
MMPortSerialAt *self = MM_PORT_SERIAL_AT (port);
- gboolean found;
GString *string;
+ gsize parsed_len;
+ GError *inner_error = NULL;
g_return_val_if_fail (self->priv->response_parser_fn != NULL, FALSE);
@@ -135,17 +137,29 @@ parse_response (MMPortSerial *port,
string = g_string_sized_new (response->len + 1);
g_string_append_len (string, (const char *) response->data, response->len);
- /* Parse it */
- found = self->priv->response_parser_fn (self->priv->response_parser_user_data, string, error);
-
- /* And copy it back into the response array after the parser has removed
- * matches and cleaned it up.
- */
- if (response->len)
- g_byte_array_remove_range (response, 0, response->len);
- g_byte_array_append (response, (const guint8 *) string->str, string->len);
- g_string_free (string, TRUE);
- return found;
+ /* Fully cleanup the response array, we'll consider the contents we got
+ * as the full reply that the command may expect. */
+ g_byte_array_remove_range (response, 0, response->len);
+
+ /* Parse it; returns FALSE if there is nothing we can do with this
+ * response yet. */
+ if (!self->priv->response_parser_fn (self->priv->response_parser_user_data, string, &inner_error)) {
+ /* Copy what we got back in the response buffer. */
+ g_byte_array_append (response, (const guint8 *) string->str, string->len);
+ g_string_free (string, TRUE);
+ return MM_PORT_SERIAL_RESPONSE_NONE;
+ }
+
+ /* If we got an error, propagate it without any further response string */
+ if (inner_error) {
+ g_propagate_error (error, inner_error);
+ return MM_PORT_SERIAL_RESPONSE_ERROR;
+ }
+
+ /* Otherwise, build a new GByteArray considered as parsed response */
+ parsed_len = string->len;
+ *parsed_response = g_byte_array_new_take ((guint8 *) g_string_free (string, FALSE), parsed_len);
+ return MM_PORT_SERIAL_RESPONSE_BUFFER;
}
/*****************************************************************************/
diff --git a/src/mm-port-serial-gps.c b/src/mm-port-serial-gps.c
index 306df1f..b316d05 100644
--- a/src/mm-port-serial-gps.c
+++ b/src/mm-port-serial-gps.c
@@ -68,9 +68,10 @@ remove_eval_cb (const GMatchInfo *match_info,
return FALSE;
}
-static gboolean
+static MMPortSerialResponseType
parse_response (MMPortSerial *port,
GByteArray *response,
+ GByteArray **parsed_response,
GError **error)
{
MMPortSerialGps *self = MM_PORT_SERIAL_GPS (port);
@@ -112,7 +113,7 @@ parse_response (MMPortSerial *port,
g_match_info_free (match_info);
if (!matches)
- return FALSE;
+ return MM_PORT_SERIAL_RESPONSE_NONE;
/* Remove matches */
result_len = response->len;
@@ -122,9 +123,11 @@ parse_response (MMPortSerial *port,
0, 0,
remove_eval_cb, &result_len, NULL);
+ /* Cleanup response buffer */
g_byte_array_remove_range (response, 0, response->len);
- g_byte_array_append (response, (const guint8 *) str, result_len);
- g_free (str);
+
+ /* Build parsed response */
+ *parsed_response = g_byte_array_new_take ((guint8 *)str, result_len);
return TRUE;
}
diff --git a/src/mm-port-serial-qcdm.c b/src/mm-port-serial-qcdm.c
index b63927f..7732851 100644
--- a/src/mm-port-serial-qcdm.c
+++ b/src/mm-port-serial-qcdm.c
@@ -59,10 +59,69 @@ find_qcdm_start (GByteArray *response, gsize *start)
return FALSE;
}
-static gboolean
-parse_response (MMPortSerial *port, GByteArray *response, GError **error)
+static MMPortSerialResponseType
+parse_response (MMPortSerial *port,
+ GByteArray *response,
+ GByteArray **parsed_response,
+ GError **error)
{
- return find_qcdm_start (response, NULL);
+ gsize start = 0;
+ gsize used = 0;
+ gsize unescaped_len = 0;
+ guint8 *unescaped_buffer;
+ qcdmbool more = FALSE;
+
+ /* Get the offset into the buffer of where the QCDM frame starts */
+ if (!find_qcdm_start (response, &start)) {
+ /* Discard the unparsable data right away, we do need a QCDM
+ * start, and anything that comes before it is unknown data
+ * that we'll never use. */
+ return MM_PORT_SERIAL_RESPONSE_NONE;
+ }
+
+ /* If there is anything before the start marker, remove it */
+ g_byte_array_remove_range (response, 0, start);
+ if (response->len == 0)
+ return MM_PORT_SERIAL_RESPONSE_NONE;
+
+ /* Try to decapsulate the response into a buffer */
+ unescaped_buffer = g_malloc (1024);
+ if (!dm_decapsulate_buffer ((const char *)(response->data),
+ response->len,
+ (char *)unescaped_buffer,
+ 1024,
+ &unescaped_len,
+ &used,
+ &more)) {
+ /* Report an error right away. Not being able to decapsulate a QCDM
+ * packet once we got message start marker likely means that this
+ * data that we got is not a QCDM message. */
+ g_set_error (error,
+ MM_SERIAL_ERROR,
+ MM_SERIAL_ERROR_PARSE_FAILED,
+ "Failed to unescape QCDM packet");
+ g_free (unescaped_buffer);
+ return MM_PORT_SERIAL_RESPONSE_ERROR;
+ }
+
+ if (more) {
+ /* Need more data, we leave the original byte array untouched so that
+ * we can retry later when more data arrives. */
+ g_free (unescaped_buffer);
+ return MM_PORT_SERIAL_RESPONSE_NONE;
+ }
+
+ /* Successfully decapsulated the DM command. We'll build a new byte array
+ * with the response, and leave the input buffer cleaned up. */
+ g_assert (unescaped_len <= 1024);
+ unescaped_buffer = g_realloc (unescaped_buffer, unescaped_len);
+ *parsed_response = g_byte_array_new_take (unescaped_buffer, unescaped_len);
+
+ /* Remove the data we used from the input buffer, leaving out any
+ * additional data that may already been received (e.g. from the following
+ * message). */
+ g_byte_array_remove_range (response, 0, used);
+ return MM_PORT_SERIAL_RESPONSE_BUFFER;
}
/*****************************************************************************/
@@ -83,75 +142,14 @@ serial_command_ready (MMPortSerial *port,
GAsyncResult *res,
GSimpleAsyncResult *simple)
{
- GByteArray *response_buffer;
GByteArray *response;
GError *error = NULL;
- gsize used = 0;
- gsize start = 0;
- guint8 *unescaped_buffer = NULL;
- gboolean success = FALSE;
- qcdmbool more = FALSE;
- gsize unescaped_len = 0;
-
- response_buffer = mm_port_serial_command_finish (port, res, &error);
- if (!response_buffer)
- goto out;
-
- /* Get the offset into the buffer of where the QCDM frame starts */
- start = 0;
- if (!find_qcdm_start (response_buffer, &start)) {
- error = g_error_new_literal (MM_SERIAL_ERROR,
- MM_SERIAL_ERROR_FRAME_NOT_FOUND,
- "QCDM frame start not found");
- /* Discard the unparsable data */
- used = response_buffer->len;
- goto out;
- }
-
- unescaped_buffer = g_malloc (1024);
- success = dm_decapsulate_buffer ((const char *)(response_buffer->data + start),
- response_buffer->len - start,
- (char *)unescaped_buffer,
- 1024,
- &unescaped_len,
- &used,
- &more);
- if (!success) {
- error = g_error_new_literal (MM_SERIAL_ERROR,
- MM_SERIAL_ERROR_PARSE_FAILED,
- "Failed to unescape QCDM packet");
- g_free (unescaped_buffer);
- unescaped_buffer = NULL;
- goto out;
- }
-
- if (more) {
- /* Need more data; we shouldn't have gotten here since the parse
- * function checks for the end-of-frame marker, but whatever.
- */
- error = g_error_new_literal (MM_CORE_ERROR,
- MM_CORE_ERROR_FAILED,
- "QCDM packet is not complete");
- g_free (unescaped_buffer);
- unescaped_buffer = NULL;
- goto out;
- }
- /* Successfully decapsulated the DM command */
- g_assert (error == NULL);
- g_assert (unescaped_len <= 1024);
- unescaped_buffer = g_realloc (unescaped_buffer, unescaped_len);
- response = g_byte_array_new_take (unescaped_buffer, unescaped_len);
- g_simple_async_result_set_op_res_gpointer (simple, response, (GDestroyNotify)g_byte_array_unref);
-
-out:
- if (error)
+ response = mm_port_serial_command_finish (port, res, &error);
+ if (!response)
g_simple_async_result_take_error (simple, error);
- if (start + used)
- g_byte_array_remove_range (response_buffer, 0, start + used);
- if (response_buffer)
- g_byte_array_unref (response_buffer);
-
+ else
+ g_simple_async_result_set_op_res_gpointer (simple, response, (GDestroyNotify)g_byte_array_unref);
g_simple_async_result_complete (simple);
g_object_unref (simple);
}
diff --git a/src/mm-port-serial.c b/src/mm-port-serial.c
index 53f8725..92ad481 100644
--- a/src/mm-port-serial.c
+++ b/src/mm-port-serial.c
@@ -696,10 +696,14 @@ port_serial_schedule_queue_process (MMPortSerial *self, guint timeout_ms)
static void
port_serial_got_response (MMPortSerial *self,
+ GByteArray *parsed_response,
const GError *error)
{
CommandContext *ctx;
+ /* Either one or the other, not both */
+ g_assert ((parsed_response && !error) || (!parsed_response && error));
+
if (self->priv->timeout_id) {
g_source_remove (self->priv->timeout_id);
self->priv->timeout_id = 0;
@@ -716,29 +720,15 @@ port_serial_got_response (MMPortSerial *self,
ctx = (CommandContext *) g_queue_pop_head (self->priv->queue);
if (ctx) {
- if (error) {
- /* If we're returning an error parsed in a generic way from the inputs,
- * we fully avoid returning a response bytearray. This really applies
- * only to AT, not to QCDM, so we shouldn't be worried of losing chunks
- * of the next QCDM message. And given that the caller won't get the
- * response array, we're the ones in charge of removing the processed
- * data (otherwise ERROR replies may get fed to the next response
- * parser).
- */
+ /* Complete the command context with the appropriate result */
+ if (error)
g_simple_async_result_set_from_error (ctx->result, error);
- g_byte_array_remove_range (self->priv->response, 0, self->priv->response->len);
- } else {
+ else {
if (ctx->allow_cached)
- port_serial_set_cached_reply (self, ctx->command, self->priv->response);
-
- /* Upon completion, it is a task of the caller to remove from the response
- * buffer the processed data. This may seem unnecessary in the case of AT
- * commands, as it'll remove the full received string, but the step is
- * a key thing in the case of QCDM, where we want to read just until the
- * next marker, not more. */
+ port_serial_set_cached_reply (self, ctx->command, parsed_response);
g_simple_async_result_set_op_res_gpointer (ctx->result,
- g_byte_array_ref (self->priv->response),
- (GDestroyNotify)g_byte_array_unref);
+ g_byte_array_ref (parsed_response),
+ (GDestroyNotify) g_byte_array_unref);
}
/* Don't complete in idle. We need the caller remove the response range which
@@ -767,7 +757,7 @@ port_serial_timed_out (gpointer data)
error = g_error_new_literal (MM_SERIAL_ERROR,
MM_SERIAL_ERROR_RESPONSE_TIMEOUT,
"Serial command timed out");
- port_serial_got_response (self, error);
+ port_serial_got_response (self, NULL, error);
g_error_free (error);
/* Emit a timed out signal, used by upper layers to identify a disconnected
@@ -792,7 +782,7 @@ port_serial_response_wait_cancelled (GCancellable *cancellable,
error = g_error_new_literal (MM_CORE_ERROR,
MM_CORE_ERROR_CANCELLED,
"Waiting for the reply cancelled");
- port_serial_got_response (self, error);
+ port_serial_got_response (self, NULL, error);
g_error_free (error);
}
@@ -814,18 +804,12 @@ port_serial_queue_process (gpointer data)
cached = port_serial_get_cached_reply (self, ctx->command);
if (cached) {
- /* Ensure the response array is fully empty before setting the
- * cached response. */
- if (self->priv->response->len > 0) {
- mm_warn ("(%s) response array is not empty when using cached "
- "reply, cleaning up %u bytes",
- mm_port_get_device (MM_PORT (self)),
- self->priv->response->len);
- g_byte_array_set_size (self->priv->response, 0);
- }
+ GByteArray *parsed_response;
- g_byte_array_append (self->priv->response, cached->data, cached->len);
- port_serial_got_response (self, NULL);
+ parsed_response = g_byte_array_sized_new (cached->len);
+ g_byte_array_append (parsed_response, cached->data, cached->len);
+ port_serial_got_response (self, parsed_response, NULL);
+ g_byte_array_unref (parsed_response);
return G_SOURCE_REMOVE;
}
@@ -834,7 +818,7 @@ port_serial_queue_process (gpointer data)
/* If error, report it */
if (!port_serial_process_command (self, ctx, &error)) {
- port_serial_got_response (self, error);
+ port_serial_got_response (self, NULL, error);
g_error_free (error);
return G_SOURCE_REMOVE;
}
@@ -860,7 +844,7 @@ port_serial_queue_process (gpointer data)
error = g_error_new (MM_CORE_ERROR,
MM_CORE_ERROR_CANCELLED,
"Won't wait for the reply");
- port_serial_got_response (self, error);
+ port_serial_got_response (self, NULL, error);
g_error_free (error);
return G_SOURCE_REMOVE;
}
@@ -873,16 +857,50 @@ port_serial_queue_process (gpointer data)
return G_SOURCE_REMOVE;
}
-static gboolean
-parse_response (MMPortSerial *self,
- GByteArray *response,
- GError **error)
+static void
+parse_response_buffer (MMPortSerial *self)
{
- if (MM_PORT_SERIAL_GET_CLASS (self)->parse_unsolicited)
- MM_PORT_SERIAL_GET_CLASS (self)->parse_unsolicited (self, response);
+ GError *error = NULL;
+ GByteArray *parsed_response = NULL;
- g_return_val_if_fail (MM_PORT_SERIAL_GET_CLASS (self)->parse_response, FALSE);
- return MM_PORT_SERIAL_GET_CLASS (self)->parse_response (self, response, error);
+ /* Parse unsolicited messages in the subclass.
+ *
+ * If any message found, it's processed immediately and the message is
+ * removed from the response buffer.
+ */
+ if (MM_PORT_SERIAL_GET_CLASS (self)->parse_unsolicited)
+ MM_PORT_SERIAL_GET_CLASS (self)->parse_unsolicited (self,
+ self->priv->response);
+
+ /* Parse response in the subclass.
+ *
+ * Returns TRUE either if an error is provided or if we really have the
+ * response to process. The parsed string is returned already out of the
+ * response buffer, and the response buffer is cleaned up accordingly.
+ */
+ g_assert (MM_PORT_SERIAL_GET_CLASS (self)->parse_response != NULL);
+ switch (MM_PORT_SERIAL_GET_CLASS (self)->parse_response (self,
+ self->priv->response,
+ &parsed_response,
+ &error)) {
+ case MM_PORT_SERIAL_RESPONSE_BUFFER:
+ /* We have a valid response to process */
+ g_assert (parsed_response);
+ self->priv->n_consecutive_timeouts = 0;
+ port_serial_got_response (self, parsed_response, NULL);
+ g_byte_array_unref (parsed_response);
+ break;
+ case MM_PORT_SERIAL_RESPONSE_ERROR:
+ /* We have an error to process */
+ g_assert (error);
+ self->priv->n_consecutive_timeouts = 0;
+ port_serial_got_response (self, NULL, error);
+ g_error_free (error);
+ break;
+ case MM_PORT_SERIAL_RESPONSE_NONE:
+ /* Nothing to do this time */
+ break;
+ }
}
static gboolean
@@ -954,8 +972,6 @@ common_input_available (MMPortSerial *self,
status = G_IO_STATUS_NORMAL;
}
-
-
/* If no bytes read, just wait for more data */
if (bytes_read == 0)
break;
@@ -971,15 +987,9 @@ common_input_available (MMPortSerial *self,
g_byte_array_remove_range (self->priv->response, 0, (SERIAL_BUF_SIZE / 2));
}
- /* Parse response. Returns TRUE either if an error is provided or if
- * we really have the response to process. */
- if (parse_response (self, self->priv->response, &error)) {
- /* Reset number of consecutive timeouts only here */
- self->priv->n_consecutive_timeouts = 0;
- /* Process response retrieved */
- port_serial_got_response (self, error);
- g_clear_error (&error);
- }
+ /* See if we can parse anything */
+ parse_response_buffer (self);
+
} while ( (bytes_read == SERIAL_BUF_SIZE || status == G_IO_STATUS_AGAIN)
&& (self->priv->iochannel_id > 0 || self->priv->socket_source != NULL));
diff --git a/src/mm-port-serial.h b/src/mm-port-serial.h
index 8f357ae..708e391 100644
--- a/src/mm-port-serial.h
+++ b/src/mm-port-serial.h
@@ -40,6 +40,12 @@
#define MM_PORT_SERIAL_SPEW_CONTROL "spew-control" /* Construct-only */
#define MM_PORT_SERIAL_FLASH_OK "flash-ok" /* Construct-only */
+typedef enum {
+ MM_PORT_SERIAL_RESPONSE_NONE,
+ MM_PORT_SERIAL_RESPONSE_BUFFER,
+ MM_PORT_SERIAL_RESPONSE_ERROR,
+} MMPortSerialResponseType;
+
typedef struct _MMPortSerial MMPortSerial;
typedef struct _MMPortSerialClass MMPortSerialClass;
typedef struct _MMPortSerialPrivate MMPortSerialPrivate;
@@ -58,16 +64,26 @@ struct _MMPortSerialClass {
*/
void (*parse_unsolicited) (MMPortSerial *self, GByteArray *response);
- /* Called to parse the device's response to a command or determine if the
- * response was an error response. If the response indicates an error, an
- * appropriate error should be returned in the 'error' argument. The
- * function should return FALSE if there is not enough data yet to determine
- * the device's reply (whether success *or* error), and should return TRUE
- * when the device's response has been recognized and parsed.
+ /*
+ * Called to parse the device's response to a command or determine if the
+ * response was an error response.
+ *
+ * If the response indicates an error, @MM_PORT_SERIAL_RESPONSE_ERROR will
+ * be returned and an appropriate GError set in @error.
+ *
+ * If the response indicates a valid response, @MM_PORT_SERIAL_RESPONSE_BUFFER
+ * will be returned, and a newly allocated GByteArray set in @parsed_response.
+ *
+ * If there is no response, @MM_PORT_SERIAL_RESPONSE_NONE will be returned,
+ * and neither @error nor @parsed_response will be set.
+ *
+ * The implementation is allowed to cleanup the @response byte array, e.g. to
+ * just remove 1 single response if more than one found.
*/
- gboolean (*parse_response) (MMPortSerial *self,
- GByteArray *response,
- GError **error);
+ MMPortSerialResponseType (*parse_response) (MMPortSerial *self,
+ GByteArray *response,
+ GByteArray **parsed_response,
+ GError **error);
/* Called to configure the serial port fd after it's opened. On error, should
* return FALSE and set 'error' as appropriate.
--
2.6.4
More information about the ModemManager-devel
mailing list