8:12 a.m.
Hi,
We noticed that the service directory and the statistics file are
created even if the connection is not successful because for example
user entered an incorrect key.
I think that it is because before starting the connection process,
ConnMan tries to read the statistics file in
service.c:service_connect(). Doing so, the function
__connman_stats_service_register() will create the directory and the
file if they do not exists. I saw that commit 50fb779bc95 already
tried to solved this but it still happens.
To solve this, I propose to either do the same of 50fb779bc95 in
service_connect():
- if (__connman_stats_service_register(service) == 0) {
+ if (!service->new_service &&
+
__connman_stats_service_register(service) == 0) {
Or, maybe better, why ConnMan needs to access the file that early?
Shouldn't it be done when service goes into CONFIGURATION state or
READY state for new services? If it does not break any statistics
feature, I would suggest to completely remove that piece of code from
service_connect():
- if (__connman_stats_service_register(service) == 0) {
- __connman_stats_get(service, false,
- &service->stats.data);
- __connman_stats_get(service, true,
- &service->stats_roaming.data);
- }
What do yo think?
Regards,
Jose Blanquicet
7:38 a.m.
On Fri, 2017-02-24 at 08:12 +0000, Jose Blanquicet wrote:
Or, maybe better, why ConnMan needs to access the file that early?
Shouldn't it be done when service goes into CONFIGURATION state or
READY state for new services? If it does not break any statistics
feature, I would suggest to completely remove that piece of code from
service_connect():
- if (__connman_stats_service_register(service) == 0) {
- __connman_stats_get(service, false,
- &service-
>stats.data);
- __connman_stats_get(service, true,
- &service-
>stats_roaming.data);
- }
What do yo think?
ConnMan statistics should count all the bytes sent via that service,
successful or not. Configuration state sounds about right, as then all
the bytes in the messages doing authentication are accounted for as
well. Unfortunately I think ethernet was skipping this state (or then
it was Associating, can't remember right now), but that should be
trivial to fix. We do want to have all bytes counted, as this is
especially important with oFono and cellular.
Unfortunately accounting is a bit broken at the moment. Stopping it
does not properly store the bytes seen and the only way to have ConnMan
account properly is to set up an external application to ask for
statistics. So the main part of the file writing is ok, but the logic
in service.c should be rewritte, it's now about two parts of the stats
code that push each other around. So the interaction should first be
cleared up from a service point of view, add simpler helper functions
capable of starting and stopping stat accounting. It may or may not be
beneficial to have the stats file created/opened and closed separately
from writing statistics values to it.
To fix this one, I'd start by verifying that Configuration is the
proper state to start accounting and all drivers follow this. Stopping
should happen when going back to Idle or Failure. Here the interface
going away triggers the service to stop, so something a bit smarter
needs to be done in order to pick up the last bytes sent. Same thing
happens when ConnMan is terminating, in addition the (idle) timeouts
are (currently) just removed when stopping.
Once that is done and is reliable, we'd need to upgrade to 64 bit stats
values. This is ok to change as the interface is marked experimental...
It would be very nice if you (or someone else) had the time to work on
this. I hacked a bit on this once upon a time, but realized twice that
it needs a bit more time to be solved nicely than I had on both
occasions.
Cheers,
Patrik
6:38 a.m.
Hi Patrik,
On 02/27/2017 08:38 AM, Patrik Flykt wrote:
On Fri, 2017-02-24 at 08:12 +0000, Jose Blanquicet wrote:
> Or, maybe better, why ConnMan needs to access the file that early?
> Shouldn't it be done when service goes into CONFIGURATION state or
> READY state for new services? If it does not break any statistics
> feature, I would suggest to completely remove that piece of code from
> service_connect():
>
> - if (__connman_stats_service_register(service) == 0) {
> - __connman_stats_get(service, false,
> - &service-
>> stats.data);
> - __connman_stats_get(service, true,
> - &service-
>> stats_roaming.data);
> - }
>
> What do yo think?
ConnMan statistics should count all the bytes sent via that service,
successful or not. Configuration state sounds about right, as then all
the bytes in the messages doing authentication are accounted for as
well. Unfortunately I think ethernet was skipping this state (or then
it was Associating, can't remember right now), but that should be
trivial to fix. We do want to have all bytes counted, as this is
especially important with oFono and cellular.
Unfortunately accounting is a bit broken at the moment. Stopping it
does not properly store the bytes seen and the only way to have ConnMan
account properly is to set up an external application to ask for
statistics.
Depends on how you see it :) It works exactly as we initially planned
it. That is without an external user of the stats API the counters are
not updated to keep overhead/power usage at a minimum.
Always updating the counters makes perfectly sense for me though you
need to take a close look on how to keep the overhead small.
Thanks,
Daniel
10:24 a.m.
On Tue, 2017-02-28 at 07:38 +0100, Daniel Wagner wrote:
Depends on how you see it :) It works exactly as we initially
planned it. That is without an external user of the stats API the
counters are not updated to keep overhead/power usage at a minimum.
Always updating the counters makes perfectly sense for me though you
need to take a close look on how to keep the overhead small.
The only problem right now is that stats are not recorded to file on
service disconnect. It has to do with the fact that the interface is
the first one to go, then the service is removed. Stats are
closed/saved only when service goes away, which is too late.
Cheers,
Patrik
3:41 p.m.
Hi,
On Tue, Feb 28, 2017 at 10:24 AM, Patrik Flykt
<Patrik.Flykt(a)linux.intel.com> wrote:
On Tue, 2017-02-28 at 07:38 +0100, Daniel Wagner wrote:
> Depends on how you see it :) It works exactly as we initially
> planned it. That is without an external user of the stats API the
> counters are not updated to keep overhead/power usage at a minimum.
>
> Always updating the counters makes perfectly sense for me though you
> need to take a close look on how to keep the overhead small.
The only problem right now is that stats are not recorded to file on
service disconnect. It has to do with the fact that the interface is
the first one to go, then the service is removed. Stats are
closed/saved only when service goes away, which is too late.
We noticed the issue because we saw too many folders
/var/lib/connman/wifi_* after a test session with only the statistics
file inside. I thought it could be easily resolved but, as always, all
my tests are done only for WiFi technology and sometime I miss
something.
I apologize but I have to let this for another moment or maybe for
someone with more time because I need to pass to other issues with
higher priority.
Thanks for the support!
Regards,
Jose Blanquicet
1992
days inactive
1997
days old
4 comments
3 participants
participants (3)
-
Daniel Wagner -
Jose Blanquicet -
Patrik Flykt