From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============5478551011585799565=="
MIME-Version: 1.0
From: Denis Kenzior <denkenz@gmail.com>
Subject: Re: [PATCH] src/modem.c/call_watches null pointer check
Date: Tue, 23 Oct 2018 12:40:05 -0500
Message-ID: <3f72eb6e-e37b-f7a5-a30c-a13c6110523a@gmail.com>
In-Reply-To: <CAKSBH7HMe=OGbwTE8QphcpOOsUCQj8226TTG60WeRSKN73FH4g@mail.gmail.com>
List-Id: <ofono.ofono.org>
To: ofono@ofono.org

--===============5478551011585799565==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

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=3Datom(a)entry=3D0x563b1a3f07d0,
>>> cond=3Dcond(a)entry=3DOFONO_ATOM_WATCH_CONDITION_UNREGISTERED) at
>>> src/modem.c:265
>>>      1  0x0000563b1842c32e in __ofono_atom_unregister
>>> (atom=3Datom(a)entry=3D0x563b1a3f07d0) at src/modem.c:299
>>>      2  0x0000563b1842c3ff in __ofono_atom_unregister
>>> (atom=3D0x563b1a3f07d0) at src/modem.c:296
>>>      3  flush_atoms (modem=3D0x563b1a3f1f50,
>>> new_state=3DMODEM_STATE_POWER_OFF) at src/modem.c:448
>>>      4  modem_change_state (modem=3D0x563b1a3f1f50,
>>> new_state=3DMODEM_STATE_POWER_OFF) at src/modem.c:529
>>>      5  0x0000563b1842c577 in set_powered
>>> (modem=3Dmodem(a)entry=3D0x563b1a3f1f50, powered=3Dpowered(a)entry=3D0)=
 at
>>> src/modem.c:915
>>>      6  0x0000563b1842c863 in modem_unregister
>>> (modem=3Dmodem(a)entry=3D0x563b1a3f1f50) at src/modem.c:2111
>>>      7  0x0000563b1842e6a4 in ofono_modem_remove (modem=3D0x563b1a3f1f5=
0)
>>> at src/modem.c:2177
>>>      8  0x0000563b183b05d3 in destroy_modem (data=3D0x563b1a3e50a0) 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=3D0x563b1a401630) at
>>> plugins/udevng.c:1468
>>>      11 udev_event (channel=3D<optimized out>, cond=3D<optimized out>,
>>> user_data=3D<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=3D<optimized out>,
>>> argv=3D<optimized out>) at src/main.c:294
>>>
>>> ?
>>
>> Yes, much better.  So now you can explain to me how you're triggering th=
is.
>>
>> 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 =3D __ofono_modem_find_atom(modem,OFONO_ATOM_TYPE_LOCATION_REP=
ORTING);
>     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

--===============5478551011585799565==--