[PATCH] port-serial: remove response buffer when an error is returned

[Dan Williams]

On Sun, 2016-01-17 at 00:55 +0100, Aleksander Morgado wrote:
> When valid responses were returned to the caller of the serial
> command, the
> caller itself was responsible for removing from the GByteArray the
> data that
> it was successfully processed (all the data in AT, just 1 message in
> QCDM). But,
> the same logic was missing for the case of errors; we were not
> explicitly
> removing the response and therefore in some cases we would see it
> propagated
> into the next command response. It was common to see this issue when
> the echo
> removal was disabled in the serial port, as in Option/HSO modems:

We don't need 'error' for the MMPortSerial parse_response class
function right?  Nothing seems to use it.  That might make this patch
clearer.

So really the only place that port_serial_got_response() would get an
error would be from internal reasons (timeout, cancellation, cached
responses, and failure to send command), never from errors from the
modem itself.  And in those cases, we blow away the buffer, right?

So wouldn't that also blow away the next messages in the buffer for
something like cancellation or random internal MM errors?

This part of the code is a bit hard to follow, unfortunately :(

Dan

>     <debug> (ttyHS3): --> 'AT+CNUM<CR>'
>     <debug> (ttyHS3): <-- '<CR><LF>+CME ERROR: 14<CR><LF>'
>     <debug> Got failure code 14: SIM busy
>     <debug> (ttyHS3) device open count is 1 (close)
>     <warn>  couldn't load list of Own Numbers: 'Failed to parse NV
> MDN command result: -17'
>     <debug> (ttyHS3) device open count is 2 (open)
>     <debug> (ttyHS3): --> 'AT_OPSYS?<CR>'
>     <debug> (ttyHS3): <-- '<CR><LF>_OPSYS:
> 1,2<CR><LF><CR><LF>OK<CR><LF>'
>     <warn>  couldn't load current allowed/preferred modes: 'Couldn't
> parse OPSYS response: '+CME ERROR: 14
>     _OPSYS: 1,2''
> ---
>  src/mm-port-serial.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mm-port-serial.c b/src/mm-port-serial.c
> index d1baa1b..53f8725 100644
> --- a/src/mm-port-serial.c
> +++ b/src/mm-port-serial.c
> @@ -716,14 +716,26 @@ port_serial_got_response (MMPortSerial *self,
>  
>      ctx = (CommandContext *) g_queue_pop_head (self->priv->queue);
>      if (ctx) {
> -        if (error)
> +        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).
> +             */
>              g_simple_async_result_set_from_error (ctx->result,
> error);
> -        else {
> -            if (ctx->allow_cached && !error)
> +            g_byte_array_remove_range (self->priv->response, 0, self
> ->priv->response->len);
> +        } 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 */
> +             * 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. */
>              g_simple_async_result_set_op_res_gpointer (ctx->result,
>                                                        
>  g_byte_array_ref (self->priv->response),
>                                                        
>  (GDestroyNotify)g_byte_array_unref);