On Fri, 2016-01-22 at 10:58 -0600, Dan Williams wrote: > 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.
Since this patch applies on top of the original "port-serial: remove response buffer when an error is returned" patch, were you thinking of pushing the two separately to record the bug fix that the first one has? Dan > 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 _______________________________________________ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel