[PATCH 2/2] bearer-mbim: parse replies on failure

[Aleksander Morgado]

On 20/12/13 08:35, Bjørn Mork wrote:
> Aleksander Morgado <aleksander at lanedo.com> writes:
> 
>> On 19/12/13 18:29, Aleksander Morgado wrote:
>>>>      if (response &&
>>>>> -        mbim_message_command_done_get_result (response, &error) &&
>>>>> +        (mbim_message_command_done_get_result (response, &error) ||
>>>>> +         error->code == MBIM_STATUS_ERROR_FAILURE) &&
>>>>>          mbim_message_connect_response_parse (
>>>>>              response,
>>>>>              &session_id,
>>>>> @@ -440,6 +441,7 @@ connect_set_ready (MbimDevice *device,
>>>>>              &nw_error,
>>>>>              &error)) {
>>>>>          if (nw_error) {
>>>>> +            g_error_free (error);
>>>>>              error = mm_mobile_equipment_error_from_mbim_nw_error (nw_error);
>>>>>              mm_dbg ("NwError: '%s' (%u)", error->message, nw_error);
>>>>>          } else {
>>> I'll rewrite a bit that logic, because there may be the case where we
>>> get an error in mbim_message_command_done_get_result() with
>>> MBIM_STATUS_ERROR_FAILURE, and *also* afterwards a
>>> mbim_message_connect_response_parse() error. Which would lead to a leak
>>> in the first GError we got.
>>
>> How about this? (attached)
> 
> Looks good to me.  I'll give a bit of testing.
> 
> But wouldn't you have saved yourself a bit of worrying if all functions
> potentinally allocating new errors, like mbim_message_connect_response_parse,
> also included something like:
> 
>  if (*error)
>     g_error_free (*error);
> 
> ?
> 

You mean within the method itself? If so, that's not what glib/gio
functions usually do. If you pass a GError pointer to be filled with an
error, the GError needs to be empty or it will dump ugly warnings.
Otherwise, how would you know if the important error was the first one
or the second one? In our case, the important one is actually the first
one, the one before the parsing error.


-- 
Aleksander