[PATCH 0/2] PPP: recording ppp dump to file

[PATCH 0/2] PPP: recording ppp dump to file

[Bertrand Aygon]

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

Create a new env var to activate PPP dump recording into a file.
Store this file in the PPP context, so the recording can occurs from
the beginning of the session, and so we are sure to not loose any frame.

Bertrand Aygon (2):
  gatppp: save dumpfile into ppp info to start recording during open
    phase.
  gprs: use a new var env to active PPP dump.

 drivers/atmodem/gprs-context.c |    5 +++++
 gatchat/gatppp.c               |   11 ++++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

-- 
1.7.4.1

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

[Bertrand Aygon]

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

---
 gatchat/gatppp.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index 2ff6c86..47a4949 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -85,6 +85,7 @@ struct _GAtPPP {
 	gboolean suspended;
 	gboolean xmit_acfc;
 	gboolean xmit_pfc;
+	gchar *dumpfile;
 };
 
 void ppp_debug(GAtPPP *ppp, const char *str)
@@ -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);
+
 	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);
 }
 
 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);
 }
 
-- 
1.7.4.1

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

[Denis Kenzior]

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

Hi Bertrand,

On 08/05/2011 03:34 AM, Bertrand Aygon wrote:
> ---
>  gatchat/gatppp.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
> index 2ff6c86..47a4949 100644
> --- a/gatchat/gatppp.c
> +++ b/gatchat/gatppp.c
> @@ -85,6 +85,7 @@ struct _GAtPPP {
>  	gboolean suspended;
>  	gboolean xmit_acfc;
>  	gboolean xmit_pfc;
> +	gchar *dumpfile;
>  };
>  
>  void ppp_debug(GAtPPP *ppp, const char *str)
> @@ -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);
> +
>  	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?

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

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

[Aygon, Bertrand]

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

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.

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

> >  }
> >
> >  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,

Bertrand
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

[Denis Kenzior]

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

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

[Aygon, Bertrand]

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

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.

Regards,

Bertrand
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

[Denis Kenzior]

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

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

[Aygon, Bertrand]

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

Hi,

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

Good compromise :)

Regards,

Bertrand
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 2/2] gprs: use a new var env to active PPP dump.

[Bertrand Aygon]

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

---
 drivers/atmodem/gprs-context.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index ae6f06a..5b2491b 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -134,6 +134,7 @@ static gboolean setup_ppp(struct ofono_gprs_context *gc)
 {
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 	GAtIO *io;
+	char *dumpfile;
 
 	DBG("");
 
@@ -152,6 +153,10 @@ static gboolean setup_ppp(struct ofono_gprs_context *gc)
 	if (getenv("OFONO_PPP_DEBUG"))
 		g_at_ppp_set_debug(gcd->ppp, ppp_debug, "PPP");
 
+	dumpfile = getenv("OFONO_PPP_DUMP");
+	if (dumpfile)
+		g_at_ppp_set_recording(gcd->ppp, dumpfile);
+
 	g_at_ppp_set_credentials(gcd->ppp, gcd->username, gcd->password);
 
 	/* set connect and disconnect callbacks */
-- 
1.7.4.1

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.