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

[Aleksander Morgado]

On 17/01/16 16:55, Dan Williams 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?
> 

Not really; the error given as input in port_serial_got_response() may
also come from the response parsing in the AT implementation itself. E.g.:

 * port-serial.c: common_input_available() receives "+CME ERROR 14"
 * port-serial.c: common_input_available() calls the subclassed
parse_response().
 * port-serial-at.c: parse_response() tries to parse the text; e.g.
processes unsolicited messages or converts the +CME ERROR string into a
GError.
 * port-serial.c: common_input_available() now calls
port_serial_got_response() passing down the processed response or the
GError.
 * port-serial.c: port_serial_got_response() sets the result (GByteArray
or GError) in the command context, which is what it's returned in the
command_finish().

So, the "+CME ERROR 14" string is converted into a GError in
port-serial-at.c: parse_response(); and that is what arrives in
port-serial.c: port_serial_got_response(). Once that happens, we can
remove the error text response from the GByteArray, which is what the
patch does, as the GByteArray is not passed as response and therefore
the caller will not clear it up.

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

If we've got unsolicited messages interleaved with the response, those
should have been processed in the subclassed parse_response(), at the
same time as we build the possible GError; so I don't think that should
affect anything.

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

It is, yes, I hope the list of steps above clarifies a bit how it goes.

-- 
Aleksander
https://aleksander.es