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: 2555 bytes --]

Hi Bertrand,

On 08/05/2011 10:36 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...
> 
> Agreed.
> 
>>>>>  	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.
> 
> Up to you to decide. If you think that we will not lose any frames if you do the set_recording just after the open, let's do it this way. I agree that it will save some memory, and so it's better.
> 

Please do it this way:

g_at_ppp_set_recording:
	if (ppp->hdlc == NULL)
		ppp->recording_filename = g_strdup(filename);
		return;

	g_at_hdlc_set_recording(filename);

and whenever you open hdlc, free recording_filename and set it to NULL
after calling g_at_hdlc_set_recording.

Free in unref as well if for some reason we fail to open/listen to to
the ppp object.

Regards,
-Denis