Re: [PATCH 1/2] gatppp: save dumpfile into ppp info to start recording during open phase.

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

Hi Bertrand,

On 08/05/2011 10:06 AM, Aygon, Bertrand wrote:
> Hi Denis,
> 
>>> @@ -541,6 +542,8 @@ gboolean g_at_ppp_open(GAtPPP *ppp, GAtIO *io)
>>>  	if (ppp->hdlc == NULL)
>>>  		return FALSE;
>>>
>>> +	g_at_hdlc_set_recording(ppp->hdlc, ppp->dumpfile);
>>> +
> 
> I force the start of the recording in the ppp_open.
> 
> Without this change, if we call g_at_ppp_set_record before g_at_ppp_open, as it's done in gsmdial, it's totaly useless. g_at_ppp_set_record will call g_at_hdlc_set_recording, and g_at_hdlc_set_recording will check ppp->hdlc. But ppp->hdlc is set in g_at_ppp_open.
>

Then at least do this everywhere g_at_hdlc is created...

>>>  	ppp->suspended = FALSE;
>>>  	g_at_hdlc_set_receive(ppp->hdlc, ppp_receive, ppp);
>>>  	g_at_hdlc_set_suspend_function(ppp->hdlc,
>>> @@ -593,7 +596,11 @@ void g_at_ppp_set_recording(GAtPPP *ppp, const
>> char *filename)
>>>  	if (ppp == NULL)
>>>  		return;
>>>
>>> -	g_at_hdlc_set_recording(ppp->hdlc, filename);
>>> +	g_free(ppp->dumpfile);
>>> +
>>> +	ppp->dumpfile = g_strdup(filename);
>>> +
>>> +	g_at_hdlc_set_recording(ppp->hdlc, ppp->dumpfile);
>>
>> I'm failing to see the point in making this change.  Why do you need to
>> save the dump filename inside gatppp?
> 
> So in the g_at_ppp_set_recording, I store the file. And during the ppp_open, I start the recording immediately. So with this change, we cannot miss a PPP frame. Because without the change, if we start the recording after the ppp_open, we might loose some info.
> 

However, I'm still not convinced we want to pay the price of storing the
filename.  Setting recording after ppp_open should be fine, all GAtHDLC
writes are deferred until the next poll/select fires, which implies
going into the main event loop.  So no data will be lost.

>>>  }
>>>
>>>  void g_at_ppp_set_connect_function(GAtPPP *ppp, GAtPPPConnectFunc
>> func,
>>> @@ -745,6 +752,8 @@ void g_at_ppp_unref(GAtPPP *ppp)
>>>
>>>  	g_at_hdlc_unref(ppp->hdlc);
>>>
>>> +	g_free(ppp->dumpfile);
>>> +
>>>  	g_free(ppp);
>>>  }
>>>

Regards,
-Denis