Re: [PATCH] src/modem.c/call_watches null pointer check

[Giacinto Cifelli <gciofono@gmail.com>]

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

Hi Denis,

On Tue, Oct 23, 2018 at 7:40 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
>
> On 10/23/2018 12:30 PM, Giacinto Cifelli wrote:
> > Hi Denis,
> >
> > I didn't know this was a surprise interrogation!
>
> That's how we roll around here.  Keeps you on your toes.
>
> >
> > On Tue, Oct 23, 2018 at 6:50 PM Denis Kenzior <denkenz@gmail.com> wrote:
> >>
> >> Hi Giacinto,
> >>>      0  0x0000563b1842b735 in call_watches
> >>> (atom=atom(a)entry=0x563b1a3f07d0,
> >>> cond=cond(a)entry=OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) at
> >>> src/modem.c:265
> >>>      1  0x0000563b1842c32e in __ofono_atom_unregister
> >>> (atom=atom(a)entry=0x563b1a3f07d0) at src/modem.c:299
> >>>      2  0x0000563b1842c3ff in __ofono_atom_unregister
> >>> (atom=0x563b1a3f07d0) at src/modem.c:296
> >>>      3  flush_atoms (modem=0x563b1a3f1f50,
> >>> new_state=MODEM_STATE_POWER_OFF) at src/modem.c:448
> >>>      4  modem_change_state (modem=0x563b1a3f1f50,
> >>> new_state=MODEM_STATE_POWER_OFF) at src/modem.c:529
> >>>      5  0x0000563b1842c577 in set_powered
> >>> (modem=modem(a)entry=0x563b1a3f1f50, powered=powered(a)entry=0) at
> >>> src/modem.c:915
> >>>      6  0x0000563b1842c863 in modem_unregister
> >>> (modem=modem(a)entry=0x563b1a3f1f50) at src/modem.c:2111
> >>>      7  0x0000563b1842e6a4 in ofono_modem_remove (modem=0x563b1a3f1f50)
> >>> at src/modem.c:2177
> >>>      8  0x0000563b183b05d3 in destroy_modem (data=0x563b1a3e50a0) at
> >>> plugins/udevng.c:1408
> >>>      9  0x00007f42b4bdd091 in ?? () from
> >>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >>>      10 0x0000563b183b561a in remove_device (device=0x563b1a401630) at
> >>> plugins/udevng.c:1468
> >>>      11 udev_event (channel=<optimized out>, cond=<optimized out>,
> >>> user_data=<optimized out>) at plugins/udevng.c:1991
> >>>      12 0x00007f42b4bef0f5 in g_main_context_dispatch () from
> >>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >>>      13 0x00007f42b4bef4c0 in ?? () from
> >>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >>>      14 0x00007f42b4bef7d2 in g_main_loop_run () from
> >>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >>>      15 0x0000563b183b0397 in main (argc=<optimized out>,
> >>> argv=<optimized out>) at src/main.c:294
> >>>
> >>> ?
> >>
> >> Yes, much better.  So now you can explain to me how you're triggering this.
> >>
> >> The only way modem->atom_watches is NULL is if ofono_modem_register
> >> failed.  So are you trying to use a modem object without registering it
> >> properly?
> >
> > the problem is that I have moved location_information_create, along
> > with devinfo_create at the gemalto_enable() phase, because these two
> > are available also offline, and it is a normal use case to monitor the
> > GNSS signal while offline.
>
> Well you can't do that.  .enable() callback is to power the modem up or
> open up the AT command port and brings the modem to RX/TX circuits off
> stage.  So you cannot be adding any atoms until
> ofono_modem_set_powered(modem, TRUE) has been called.
>
> In fact you should not be calling any atom creation functions outside
> the pre_sim, post_sim or post_online callbacks.  There may be one or two
> exceptions due to modems taking a while to initialize the SMS/phonebook
> store, and we still need to fix that.
>
> However, it still doesn't explain to me why modem->atom_watches are NULL
> in the first place.  Are you calling ofono_modem_registered first?
>
> > But when the modem is removed (unplugged or switched off), the
> > location_reporting atom is not removed, and the modem does not
> > disappear. devinfo goes away without any issue.
> >
> > So in the gemalto_remove I have added:
> >     lratom = __ofono_modem_find_atom(modem,OFONO_ATOM_TYPE_LOCATION_REPORTING);
> >     if (lratom)
> >         __ofono_atom_free(lratom);
>
> This tells me you're doing something very wrong...
>
> >
> > this works fine, but maybe the watch is already destroyed?
> >
> > And the whole point is, I wouldn't have to do it manually. What is
> > missing in this atom?
>
> Nothing is missing, all atoms are managed automagically.
>
> Regards,
> -Denis

ok, I keep it in pre-sim.

Regards,
Giacinto