Cancel set_online request if modem is powered off. JB#61333 #62
Conversation
pvuorela
commented
Mar 22, 2024
pvuorela
commented
Doc package change done long time ago and installations have migrated. Also rpm from 2013 is no longer 'reasonably fresh'.
Had Android device paired via bluetooth to Sailfish one, then disconnected them. A hfp modem was created which stayed offline. That's a problem of its own, but amplified by connman trying every 2 seconds to make the modem go online. That's doomed to fail as long as it's not powered. Thus, if set_powered fails, the online attempts are stopped.
pvuorela
force-pushed
the
stop_online_requests
branch
from
8c9bcdb
to
32d5319
Compare
| @@ -1197,6 +1224,10 @@ static void modem_delete(gpointer value) | |||
| g_source_remove(md->online_check_id); | |||
| } | |||
|
|
|||
| if (md->pending_set_powered) { | |||
| g_cancellable_cancel(md->pending_set_powered); | |||
There was a problem hiding this comment.
Cancellation happens asynchronously, via separate mainloop dispatch. I suspect that that can lead to call to set_powered_cb(), which will be poking 'md' object already freed below. So, the callback might need to check that it is not dealing with G_IO_ERROR_CANCELLED error or stte before dereferencing md arg.
But it might just be me being confused with how all this cancellation stuff works in general / in context of libgofono, and this matches how other places in connman deal with similar cancellations -> I guess it is okay?
There was a problem hiding this comment.
Well in the core the g_cancellable_cancel is not used. Only in the plugins this is used so I guess it is ok to use here. But yeah, I was wondering that does this call the cb still for the cleanup? Apparently it can.
| g_cancellable_cancel(md->pending_set_powered); | ||
| } | ||
| md->pending_set_powered | ||
| = ofono_modem_set_powered_full(modem, TRUE, |
There was a problem hiding this comment.
I'm just wondering that the process kind of gets twisted here. Now we're waiting for a callback to finish but that callback would set the online check off. And regardless of the state we start the online check. For me it would be kind of making sense that we do nothing until we get the verification that the powering was successful. I.e., do the online checking later, not sure if that will break anything but this kind of breaks how it used to work - despite the fact that the success of the action was ignored and was kind of hoping it will succeed.
If the powering does not succeed the online check should not be started. I'd like to try this one out a bit with the online check initiated only after the callback gets a-ok from ofono. What do you think?
There was a problem hiding this comment.
Now we're waiting for a callback to finish
AFAICT there is no change in waiting. There is and was async power on, async online set with timer based online checking. The difference being that the latter part gets stopped after the fact if initial power on failed.
If the powering does not succeed the online check should not be started. I'd like to try this one out a bit with the online check initiated only after the callback gets a-ok from ofono. What do you think?
Sounds like it should be this way. But then the same probably would apply for the online set too i.e. wait for result of set instead of timer based poll/retry?
There was a problem hiding this comment.
Yes and no, I guess.
On one hand this could be waiting for the result before proceeding with the online state. On the other hand the logic split into numerous callbacks for async operations, each one proceeding a bit forward, tends to be tedious to follow. If it's just set_powered + set_online, then on the happy path it shouldn't make a difference, and on the error case the failed set_online shouldn't really matter much either. So if the logic doesn't require splitting into steps, I'd rather not go there.
Also to note I didn't really make logic changes here on the common setup, merely just stopping the repeating failed calls when it's known to keep failing.
Whether the create_modem() case should postpone some setup parts to be done after calling both set_powered and set_online is another topic, getting deeper in the details of what's really going on. Frankly I'm not too excited on going there right now, but I'm happy to hand over that part if you feel like it :) I'm myself already content that connman doesn't keep doing silly things every 2 seconds.
There was a problem hiding this comment.
AFAICT there is no change in waiting. There is and was async power on, async online set with timer based online checking. The difference being that the latter part gets stopped after the fact if initial power on failed.
I think I picked the wrong word here. But we are waiting the async to finish to then later decide the fate of the online check.
Sounds like it should be this way. But then the same probably would apply for the online set too i.e. wait for result of set instead of timer based poll/retry?
Yeah, it would be the optimal situation to rely always on what ofono has done and reported back. We'd need timers only for doing this repeatedly as the modems may crash. EDIT: since I think it was the original idea of how this should work in error cases.
| @@ -1197,6 +1224,10 @@ static void modem_delete(gpointer value) | |||
| g_source_remove(md->online_check_id); | |||
| } | |||
|
|
|||
| if (md->pending_set_powered) { | |||
| g_cancellable_cancel(md->pending_set_powered); | |||
There was a problem hiding this comment.
Well in the core the g_cancellable_cancel is not used. Only in the plugins this is used so I guess it is ok to use here. But yeah, I was wondering that does this call the cb still for the cleanup? Apparently it can.
| @@ -1177,7 +1202,9 @@ static void modem_create(struct plugin_data *plugin, OfonoModem *modem) | |||
| g_hash_table_replace(plugin->modems, g_strdup(path), md); | |||
|
|
|||
| if (ofono_modem_valid(modem)) { | |||
| ofono_modem_set_powered(modem, TRUE); | |||
| md->pending_set_powered | |||
| = ofono_modem_set_powered_full(modem, TRUE, | |||
There was a problem hiding this comment.
Also here we're doing a lot of unnecessary extra if the modem cannot be powered on. Online check, network create/disable, context updates, network registration. Not sure what the effect is if we have an half setup modem here with online check disabled as it cannot be powered on.
There is a comment in modem_can_create_network():
/*
* Don't create the network if cellular technology is disabled,
* otherwise connman will keep on trying to connect it.
* /
This is kind of weird situation as connman should not even create a device for this case and that should block from ever trying the false modem in anyway to be powered up. Granted, this callback use is good addition to error cases but I think some blacklisting might be looked at as well...
