On 26/06/14 11:12, Tomasz Bursztyka wrote:
Hi,
- connman_agent_cancel(service);
>+ __connman_wispr_stop(service);
Just adding __connman_wispr_stop() should do the trick here.
It could. However, if connman_agent_cancel() must be invoked every
time __connman_wispr_stop() is called then it should be impossible to
call __connman_wispr_stop() without calling connman_agent_cancel().
Otherwise crashes likes this will keep happening. This one cost me at
least a day of work. I don't want to do it again.
You are assuming that agent and wispr are tight together, which is not
exactly true. Link is indirect.
If the logic is broken, it will be in service.c: that one starts wispr
(no matter whatever agent
request is running or not), wispr then might start an agent request
(browser stuff): still the request
is tighten to service. Thus it's always up to service to cancel any
agent request.
There is not much place where __connman_wispr_stop() is called in
service.c
And actually, what your patch does is calling it earlier than in
service_indicate_state() with state
CONNMAN_SERVICE_STATE_DISCONNECT as it is done currently. Which I
think you are right here.
So a proper patch would be to remove this call in
service_indicate_state() and put the one you
want in __connman_service_disconnect()
Also, it is called in service_free() but not connman_agent_cancel(),
which in this case might be
another side of the bug. Adding connman_agent_cancel() here would be
relevant I guess.
Not sure if that can be easily tested though.
Verify that but I think it looks like the proper way to do it.
Nice catch anyway
Tomasz
You haven't convinced me. If service doesn't start those requests then
it's not its responsibility to cancel them. By doing so it would make an
unnecessary assumption about the inner workings of the wispr module. If
the wispr implementation changes, this assumption may become invalid.
The requests that need to be canceled when wispr context is destroyed,
are those started from the wispr code. And therefore it's the wispr code
which has to cancel them. The fewer assumptions and inter-dependencies
the better.
I'm not 100% happy with my solution either. Ideally I would like to be
able to cancel individual connection agent requests rather than
everything related to the same service. Otherwise destroying wispr
context may affect something totally unrelated (like passphrase input).
But there doesn't seem to be any API for that.
-Slava
_______________________________________________
connman mailing list
[email protected]
https://lists.connman.net/mailman/listinfo/connman
