On Fri, 2016-01-22 at 16:30 +0100, Aleksander Morgado wrote:
> 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 :)
Patch looks good to me, I actually have no comments at all, and it
seemed to work OK on 3 modems (including 2 with QCDM) :) Nice cleanup.
Dan
> ---
> 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
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
