cashew: support "usage requests allowed OTA only" policy

cashew: support "usage requests allowed OTA only" policy

BUG=chromium-os:7769
TEST=Manual: connect to 3G and observe logs while unplugging/plugging
ethernet

Change-Id: I70498abe97b1eb85e3bd6cc8ae20ecda6f7650b9

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=ab45a69

[Vince Laviano, 2010-10-19 03:31:04]

This CL extends cashew to support a policy of only sending usage requests over
the air (and makes this the default policy).

cashew now pays attention to considerably more flimflam state: global flimflam
connectivity state, individual service connectivity state, default technology,
and the default service (as implicitly communicated by the ordering of the
Services list).

Each service object uses these inputs in conjunction with its policy to
determine whether or not it should be sending usage updates at any given time.
This decision-making is centralized in method ShouldSendUsageRequests, called
from method ReconsiderSendingUsageRequests. Each of the service's many event
handlers (OnFoo or OnFooUpdate) calls ReconsiderSendingUsageRequests if
something significant has changed that could cause the service to want to start
or stop sending usage API requests.

[Eric Shienbrood, 2010-10-19 20:14:16]

Just one other question: What do you have against std::string::operator== ?

http://codereview.chromium.org/3764012/diff/1/5
File src/service.cc (right):

http://codereview.chromium.org/3764012/diff/1/5#newcode101
src/service.cc:101: if (type.compare(kFlimflamServiceTypeEthernet) == 0) {
Why do this rather than just doing the simpler "if (type ==
kFlimflamServiceTypeEthernet)"?

http://codereview.chromium.org/3764012/diff/1/5#newcode134
src/service.cc:134: bool Service::IsDefaultService() const {
Note - I will be checking in a flimflam change soon that adds an IsDefault
property to services. As its name implies, it will be set to true for the
service (if any) which is the default. This should allow you to simplify your
code quite a bit.

http://codereview.chromium.org/3764012/diff/1/5#newcode644
src/service.cc:644: << ": IsConnected: service manager thinks we're offline";
Do you want to return false here?

http://codereview.chromium.org/3764012/diff/1/5#newcode692
src/service.cc:692: if (!IsDefaultService()) {
We are eventually going to have to solve this problem by providing a way to
route selected traffic through a particular network interface. If there's not
already a bug filed for this, I'll create one.

http://codereview.chromium.org/3764012/diff/1/7
File src/service_manager.cc (right):

http://codereview.chromium.org/3764012/diff/1/7#newcode262
src/service_manager.cc:262: // to know about this, since it means that there is
no default service.
But there might be no default service even if the list of services is not empty.

[Vince Laviano, 2010-10-19 21:52:08]

http://codereview.chromium.org/3764012/diff/1/5
File src/service.cc (right):

http://codereview.chromium.org/3764012/diff/1/5#newcode101
src/service.cc:101: if (type.compare(kFlimflamServiceTypeEthernet) == 0) {
On 2010/10/19 20:14:16, Eric Shienbrood wrote:
> Why do this rather than just doing the simpler "if (type ==
> kFlimflamServiceTypeEthernet)"?

Fixed. (Here and elsewhere in cashew)

The answer is that I started out using std::string::compare and then shifted
over to using std::string::operator== in newer code. However, this is older code
that is being moved from being private to being public.

What do I have against std::string::operator==?

Nothing, but I'm not a big fan of operator overloading in general and so I tend
towards the alternatives when they are available.

I find it disconcerting that a + b can do just about anything (database lookups,
send people email, read the contents of the Library of Congress and become
self-aware). It's easy for someone unfamiliar with a code base and with
preconceptions about what a particular operator does to read past this and have
it totally escape them. They are more likely to notice and investigate an
invocation of a.Add(b) to ensure that they understand what it's doing.

http://codereview.chromium.org/3764012/diff/1/5#newcode134
src/service.cc:134: bool Service::IsDefaultService() const {
On 2010/10/19 20:14:16, Eric Shienbrood wrote:
> Note - I will be checking in a flimflam change soon that adds an IsDefault
> property to services. As its name implies, it will be set to true for the
> service (if any) which is the default. This should allow you to simplify your
> code quite a bit.

Great. I look forward to it. Please cc me on the CL.

http://codereview.chromium.org/3764012/diff/1/5#newcode644
src/service.cc:644: << ": IsConnected: service manager thinks we're offline";
On 2010/10/19 20:14:16, Eric Shienbrood wrote:
> Do you want to return false here?

Done.

In an earlier version of the code, there was an issue with the ordering in which
Flimflam sends PropertyChanged and StateChanged signals that would cause us to
get stuck thinking that a service is offline when it's not, so I demoted this to
just a warning.  Adding the OnFlimflamOnline() and OnFlimflamOffline() event
notifications solved the issue, but I didn't update the code here.

Thanks for catching it.

http://codereview.chromium.org/3764012/diff/1/5#newcode692
src/service.cc:692: if (!IsDefaultService()) {
On 2010/10/19 20:14:16, Eric Shienbrood wrote:
> We are eventually going to have to solve this problem by providing a way to
> route selected traffic through a particular network interface. If there's not
> already a bug filed for this, I'll create one.

Sounds good. I'm not aware of an existing bug for this.

http://codereview.chromium.org/3764012/diff/1/7
File src/service_manager.cc (right):

http://codereview.chromium.org/3764012/diff/1/7#newcode262
src/service_manager.cc:262: // to know about this, since it means that there is
no default service.
On 2010/10/19 20:14:16, Eric Shienbrood wrote:
> But there might be no default service even if the list of services is not
empty.

That's probably ok, since, looking at the get_default function in flimflam's
service.c, the only way for this to happen is if no services are ready.  In the
ota-only policy case, we consider not only whether we think it's the default but
also if it's connected (i.e. in ready state), so we wouldn't send a request in
this scenario.

The IsDefault property that you mentioned in your other comment should solve
this problem more cleanly.

[Jason Glasgow, 2010-10-19 22:56:49]

http://codereview.chromium.org/3764012/diff/6001/7006
File src/service.cc (right):

http://codereview.chromium.org/3764012/diff/6001/7006#newcode338
src/service.cc:338: // we just stopped being the default service
Why the else clause with nothing but a comment?

http://codereview.chromium.org/3764012/diff/6001/7009
File src/service_manager.h (right):

http://codereview.chromium.org/3764012/diff/6001/7009#newcode54
src/service_manager.h:54: kConnectivityStateConnected,
What is the difference between Connected and Online?

http://codereview.chromium.org/3764012/diff/6001/7009#newcode121
src/service_manager.h:121: void OnDefaultServiceUpdate(const DBus::Path *path);
Why not a const reference?  I think that is the Google style.

[Vince Laviano, 2010-10-19 23:32:15]

http://codereview.chromium.org/3764012/diff/6001/7006
File src/service.cc (right):

http://codereview.chromium.org/3764012/diff/6001/7006#newcode338
src/service.cc:338: // we just stopped being the default service
On 2010/10/19 22:56:49, Jason Glasgow wrote:
> Why the else clause with nothing but a comment?
> 

Removed.

There used to be code there, and I was leaving it as documentation.

http://codereview.chromium.org/3764012/diff/6001/7009
File src/service_manager.h (right):

http://codereview.chromium.org/3764012/diff/6001/7009#newcode54
src/service_manager.h:54: kConnectivityStateConnected,
On 2010/10/19 22:56:49, Jason Glasgow wrote:
> What is the difference between Connected and Online?

Offline and Online are the usual states. According to the flimflam docs: "In
certain situations the state might change to the value "connected". This can
only be seen if previously no connection was present." I've never seen this, but
I decided to support it just in case.

http://codereview.chromium.org/3764012/diff/6001/7009#newcode121
src/service_manager.h:121: void OnDefaultServiceUpdate(const DBus::Path *path);
On 2010/10/19 22:56:49, Jason Glasgow wrote:
> Why not a const reference?  I think that is the Google style.

This method expects the caller to use path == NULL to represent the case where
the list of service paths is empty. A reference wouldn't allow this. We could
use path == "" to represent the empty list, but it's less clean in that the
empty pathname is logically distinct from an absent pathname.

Putting on my rules lawyer hat and checking out the style guide, it recommends
const references or values for input args but doesn't require them, saying
"Input parameters may be const pointers, but we never allow non-const reference
parameters."

[Jason Glasgow, 2010-10-19 23:36:00]

LGTM

[Eric Shienbrood, 2010-10-20 02:13:53]

LGTM

http://codereview.chromium.org/3764012/diff/1/5
File src/service.cc (right):

http://codereview.chromium.org/3764012/diff/1/5#newcode101
src/service.cc:101: if (type.compare(kFlimflamServiceTypeEthernet) == 0) {
On 2010/10/19 21:52:09, Vince Laviano wrote:
> On 2010/10/19 20:14:16, Eric Shienbrood wrote:
> > Why do this rather than just doing the simpler "if (type ==
> > kFlimflamServiceTypeEthernet)"?
> 
> Fixed. (Here and elsewhere in cashew)
> 
> The answer is that I started out using std::string::compare and then shifted
> over to using std::string::operator== in newer code. However, this is older
code
> that is being moved from being private to being public.
> 
> What do I have against std::string::operator==?
> 
> Nothing, but I'm not a big fan of operator overloading in general and so I
tend
> towards the alternatives when they are available.
> 
> I find it disconcerting that a + b can do just about anything (database
lookups,
> send people email, read the contents of the Library of Congress and become
> self-aware). It's easy for someone unfamiliar with a code base and with
> preconceptions about what a particular operator does to read past this and
have
> it totally escape them. They are more likely to notice and investigate an
> invocation of a.Add(b) to ensure that they understand what it's doing.

I agree, and that's why our style guide prohibits operator overloading in
classes we define. But like many features in C++, if you use it in a suitably
constrained way, it can enhance rather than detract from code comprehensibility.
And string comparison is probably the canonical example of an appropriate use of
operator overloading.