* [PATCH 1/2] gatppp: Check ppp instance before unref it @ 2010-07-06 9:57 Zhenhua Zhang 2010-07-06 9:57 ` [PATCH 2/2] gathdlc: Unset disconnect function in unref Zhenhua Zhang 2010-07-06 12:48 ` [PATCH 1/2] gatppp: Check ppp instance before unref it Marcel Holtmann 0 siblings, 2 replies; 7+ messages in thread From: Zhenhua Zhang @ 2010-07-06 9:57 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 435 bytes --] --- gatchat/gatppp.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c index 1d41ded..d9b1627 100644 --- a/gatchat/gatppp.c +++ b/gatchat/gatppp.c @@ -446,6 +446,9 @@ void g_at_ppp_unref(GAtPPP *ppp) { gboolean is_zero; + if (ppp == NULL) + return; + is_zero = g_atomic_int_dec_and_test(&ppp->ref_count); if (is_zero == FALSE) -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] gathdlc: Unset disconnect function in unref 2010-07-06 9:57 [PATCH 1/2] gatppp: Check ppp instance before unref it Zhenhua Zhang @ 2010-07-06 9:57 ` Zhenhua Zhang 2010-07-06 10:03 ` Zhang, Zhenhua 2010-07-06 16:28 ` Denis Kenzior 2010-07-06 12:48 ` [PATCH 1/2] gatppp: Check ppp instance before unref it Marcel Holtmann 1 sibling, 2 replies; 7+ messages in thread From: Zhenhua Zhang @ 2010-07-06 9:57 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 531 bytes --] If we destroy PPP instance from g_at_ppp_unref, we should not invoke io disconnect function. --- gatchat/gathdlc.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c index 08a1939..0186e46 100644 --- a/gatchat/gathdlc.c +++ b/gatchat/gathdlc.c @@ -278,6 +278,7 @@ void g_at_hdlc_unref(GAtHDLC *hdlc) hdlc->record_fd = -1; } + g_at_io_set_disconnect_function(hdlc->io, NULL, NULL); g_at_io_unref(hdlc->io); hdlc->io = NULL; -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH 2/2] gathdlc: Unset disconnect function in unref 2010-07-06 9:57 ` [PATCH 2/2] gathdlc: Unset disconnect function in unref Zhenhua Zhang @ 2010-07-06 10:03 ` Zhang, Zhenhua 2010-07-06 16:45 ` Denis Kenzior 2010-07-06 16:28 ` Denis Kenzior 1 sibling, 1 reply; 7+ messages in thread From: Zhang, Zhenhua @ 2010-07-06 10:03 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 2696 bytes --] Hi, Zhenhua Zhang wrote: > If we destroy PPP instance from g_at_ppp_unref, we should not invoke > io disconnect function. --- > gatchat/gathdlc.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/gatchat/gathdlc.c b/gatchat/gathdlc.c > index 08a1939..0186e46 100644 > --- a/gatchat/gathdlc.c > +++ b/gatchat/gathdlc.c > @@ -278,6 +278,7 @@ void g_at_hdlc_unref(GAtHDLC *hdlc) > hdlc->record_fd = -1; } > > + g_at_io_set_disconnect_function(hdlc->io, NULL, NULL); > g_at_io_unref(hdlc->io); hdlc->io = NULL; Here the problem is that disconnect function could be invoked if remote IO is disconnected after our ppp instance is freed. So we should avoid to invoke io_disconnect() in below case. Entering new phase: 5 PPP: lcp: pppcp_timeout: current state 5:STOPPING PPP: lcp: pppcp_generate_event: current state 5:STOPPING PPP: event: 5 (TO-), action: 803, new_state: 3 (STOPPED) PPP: lcp: pppcp_this_layer_finished: current state 3:STOPPED Entering new phase: 0 ofonod[3114]: src/emulator.c:ppp_disconnect() Server: > \r\nNO CARRIER\r\n ofonod[3114]: Pcui:< \r\n^BOOT:20291346,0,0,0,20\r\n Server: < AT+CFUN=0\r ofonod[3114]: src/emulator.c:cfun_cb() set CFUN to 0 Server: > \r\nOK\r\n (gdb) bt #0 0x0806faf6 in pppcp_generate_event (data=0x95bfde0, event_type=DOWN, packet=0x0, len=0) at gatchat/ppp_cp.c:638 #1 0x0806e9cd in io_disconnect (user_data=0x95bfde8) at gatchat/gatppp.c:349 #2 0x001ea504 in g_source_callback_unref (cb_data=0x95a8678) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:1077 #3 0x001eaace in g_source_destroy_internal (source=0x95a86b0, context=<value optimized out>, have_lock=1) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:856 #4 0x001eaedb in g_main_dispatch (context=0x95a5f80) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:1985 #5 IA__g_main_context_dispatch (context=0x95a5f80) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2513 #6 0x001ee730 in g_main_context_iterate (context=0x95a5f80, block=<value optimized out>, dispatch=1, self=0x95a6c58) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2591 #7 0x001eeb9f in IA__g_main_loop_run (loop=0x95a5ef0) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2799 #8 0x08094548 in main (argc=1, argv=0xbffb8ba4) at src/main.c:227 (gdb) frame 1 #1 0x0806e9cd in io_disconnect (user_data=0x95bfde8) at gatchat/gatppp.c:349 349 pppcp_signal_down(ppp->lcp); (gdb) p ppp $6 = <value optimized out> (gdb) info local No locals. (gdb) > -- > 1.6.3.3 > > _______________________________________________ > ofono mailing list > ofono(a)ofono.org > http://lists.ofono.org/listinfo/ofono Regards, Zhenhua ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] gathdlc: Unset disconnect function in unref 2010-07-06 10:03 ` Zhang, Zhenhua @ 2010-07-06 16:45 ` Denis Kenzior 0 siblings, 0 replies; 7+ messages in thread From: Denis Kenzior @ 2010-07-06 16:45 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 2241 bytes --] Hi Zhenhua, > Here the problem is that disconnect function could be invoked if remote IO is disconnected after our ppp instance is freed. So we should avoid to invoke io_disconnect() in below case. > > Entering new phase: 5 > PPP: lcp: pppcp_timeout: current state 5:STOPPING > PPP: lcp: pppcp_generate_event: current state 5:STOPPING > PPP: event: 5 (TO-), action: 803, new_state: 3 (STOPPED) > PPP: lcp: pppcp_this_layer_finished: current state 3:STOPPED > Entering new phase: 0 > ofonod[3114]: src/emulator.c:ppp_disconnect() > Server: > \r\nNO CARRIER\r\n > ofonod[3114]: Pcui:< \r\n^BOOT:20291346,0,0,0,20\r\n > Server: < AT+CFUN=0\r > ofonod[3114]: src/emulator.c:cfun_cb() set CFUN to 0 > Server: > \r\nOK\r\n > > > (gdb) bt > #0 0x0806faf6 in pppcp_generate_event (data=0x95bfde0, event_type=DOWN, packet=0x0, len=0) > at gatchat/ppp_cp.c:638 > #1 0x0806e9cd in io_disconnect (user_data=0x95bfde8) at gatchat/gatppp.c:349 > #2 0x001ea504 in g_source_callback_unref (cb_data=0x95a8678) > at /build/buildd/glib2.0-2.22.3/glib/gmain.c:1077 > #3 0x001eaace in g_source_destroy_internal (source=0x95a86b0, context=<value optimized out>, have_lock=1) > at /build/buildd/glib2.0-2.22.3/glib/gmain.c:856 > #4 0x001eaedb in g_main_dispatch (context=0x95a5f80) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:1985 > #5 IA__g_main_context_dispatch (context=0x95a5f80) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2513 > #6 0x001ee730 in g_main_context_iterate (context=0x95a5f80, block=<value optimized out>, dispatch=1, > self=0x95a6c58) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2591 > #7 0x001eeb9f in IA__g_main_loop_run (loop=0x95a5ef0) at /build/buildd/glib2.0-2.22.3/glib/gmain.c:2799 > #8 0x08094548 in main (argc=1, argv=0xbffb8ba4) at src/main.c:227 > (gdb) frame 1 > #1 0x0806e9cd in io_disconnect (user_data=0x95bfde8) at gatchat/gatppp.c:349 > 349 pppcp_signal_down(ppp->lcp); > (gdb) p ppp > $6 = <value optimized out> > (gdb) info local > No locals. > (gdb) It seems the real culprit is that GAtServer is not restoring the io_disconnect on the GAtIO object in g_at_server_resume. Why are we not seeing this crash in test-server? Regards, -Denis ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] gathdlc: Unset disconnect function in unref 2010-07-06 9:57 ` [PATCH 2/2] gathdlc: Unset disconnect function in unref Zhenhua Zhang 2010-07-06 10:03 ` Zhang, Zhenhua @ 2010-07-06 16:28 ` Denis Kenzior 2010-07-07 1:32 ` Zhang, Zhenhua 1 sibling, 1 reply; 7+ messages in thread From: Denis Kenzior @ 2010-07-06 16:28 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 586 bytes --] Hi Zhenhua, > @@ -278,6 +278,7 @@ void g_at_hdlc_unref(GAtHDLC *hdlc) > hdlc->record_fd = -1; > } > > + g_at_io_set_disconnect_function(hdlc->io, NULL, NULL); Since GAtHDLC does not set the disconnect function, this really belongs in GAtPPP. > g_at_io_unref(hdlc->io); > hdlc->io = NULL; > However, I really question why this patch is necessary. The only way the disconnect function is not reset today is if the GAtIO is refed and g_at_chat_resume is not called. We do call g_at_io_set_disconnect function in g_at_chat_resume. Regards, -Denis ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/2] gathdlc: Unset disconnect function in unref 2010-07-06 16:28 ` Denis Kenzior @ 2010-07-07 1:32 ` Zhang, Zhenhua 0 siblings, 0 replies; 7+ messages in thread From: Zhang, Zhenhua @ 2010-07-07 1:32 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 949 bytes --] Hi Denis, Denis Kenzior wrote: > Hi Zhenhua, > >> @@ -278,6 +278,7 @@ void g_at_hdlc_unref(GAtHDLC *hdlc) >> hdlc->record_fd = -1; } >> >> + g_at_io_set_disconnect_function(hdlc->io, NULL, NULL); > > Since GAtHDLC does not set the disconnect function, this really > belongs in GAtPPP. How about unset io_disconnect() in GAtPPP? Since GAtPPP doesn't have suspend/resume, the io_disconnect always exist until ppp is dead. >> g_at_io_unref(hdlc->io); >> hdlc->io = NULL; >> > > However, I really question why this patch is necessary. The only way > the disconnect function is not reset today is if the GAtIO is refed > and g_at_chat_resume is not called. > > We do call g_at_io_set_disconnect function in g_at_chat_resume. Good catch. I fixed it in gatserver and will send a patch out soon. Yes, after reset the disconnect function, the problem is gone. > Regards, > -Denis Regards, Zhenhua ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] gatppp: Check ppp instance before unref it 2010-07-06 9:57 [PATCH 1/2] gatppp: Check ppp instance before unref it Zhenhua Zhang 2010-07-06 9:57 ` [PATCH 2/2] gathdlc: Unset disconnect function in unref Zhenhua Zhang @ 2010-07-06 12:48 ` Marcel Holtmann 1 sibling, 0 replies; 7+ messages in thread From: Marcel Holtmann @ 2010-07-06 12:48 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 697 bytes --] Hi Zhenhua, > --- > gatchat/gatppp.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c > index 1d41ded..d9b1627 100644 > --- a/gatchat/gatppp.c > +++ b/gatchat/gatppp.c > @@ -446,6 +446,9 @@ void g_at_ppp_unref(GAtPPP *ppp) > { > gboolean is_zero; > > + if (ppp == NULL) > + return; > + > is_zero = g_atomic_int_dec_and_test(&ppp->ref_count); > > if (is_zero == FALSE) since we have been safe-guarding this in other unref functions as well, I also applied this patch. In general the calling code is doing something wrong here if it tries to unref a NULL pointer. Regards Marcel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-07-07 1:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-07-06 9:57 [PATCH 1/2] gatppp: Check ppp instance before unref it Zhenhua Zhang 2010-07-06 9:57 ` [PATCH 2/2] gathdlc: Unset disconnect function in unref Zhenhua Zhang 2010-07-06 10:03 ` Zhang, Zhenhua 2010-07-06 16:45 ` Denis Kenzior 2010-07-06 16:28 ` Denis Kenzior 2010-07-07 1:32 ` Zhang, Zhenhua 2010-07-06 12:48 ` [PATCH 1/2] gatppp: Check ppp instance before unref it Marcel Holtmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.