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

[Dan Williams]

On Sun, 2016-01-17 at 18:24 +0100, Aleksander Morgado wrote:
> 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.
> 

Yeah, I missed that one use of error.  You are correct.

> 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.

I thought of a bunch of scenarios, and then figured out they weren't
relevant for this patch.  But it took a while to get there and I wish
it was clearer.

So the logic (and patch) look OK to me.

Dan