Cleanup network event logging, add to ManagedNetworkConfigurationHandler

Cleanup network event logging, add to ManagedNetworkConfigurationHandler

I noticed we were getting some ERROR spam when we request properites for
networks that are no longer visible, which required some additiona
logging to track down (and hopefully make it easier to track down other
issues in the future).

BUG=none
R=pneubeck@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258956

[pneubeck (no reviews), 2014-03-21 09:33:42]

As far as I understand your intentions, you try to prevent calls to
GetNameFromProperties if there is no type.

On a first reading however, these type-property checks are rather surprising. If
these cases occur more basic things seem to go wrong, which should be handled
and not just be ignored.

But maybe I'm missing some context here.

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/network...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/network...
chromeos/network/network_configuration_handler.cc:57: if
(properties.HasKey(shill::kTypeProperty)) {
isn't this a major error and should stop the connection attempt / call
error_callback ?

if you keep it as it is, please drop a comment what this condition is for.

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/network...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:162: if
(!properties.HasKey(shill::kTypeProperty)) {
isn't in this case the NetworkState broken and will never be "repaired"?
can we ensure that such NetworkState is destructed/removed immediately? if not
and NetworkStateHandler still calls set_update_received(), we might even use
such incomplete states.

Alternatively, instead of checking (e.g. in GetNetworkState) for
update_received, we could use a new function
  bool NetworkState::IsValid() {
    return update_received() && !type_.empty();
  }

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/shill_p...
File chromeos/network/shill_property_util.cc (right):

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/shill_p...
chromeos/network/shill_property_util.cc:128: if
(properties.GetString(shill::kGuidProperty, &result))
...WithoutPathExpansion...
(I hate that verbosity) :-/

[stevenjb, 2014-03-21 17:08:51]

ptal

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/network...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/network...
chromeos/network/network_configuration_handler.cc:57: if
(properties.HasKey(shill::kTypeProperty)) {
On 2014/03/21 09:33:42, pneubeck wrote:
> isn't this a major error and should stop the connection attempt / call
> error_callback ?
> 
> if you keep it as it is, please drop a comment what this condition is for.

This one happens regularly, but it's because sometimes we request properties for
a service that is no longer there. I'll move the logic below the call_status
check instead.

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/network...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/network...
chromeos/network/network_state.cc:162: if
(!properties.HasKey(shill::kTypeProperty)) {
On 2014/03/21 09:33:42, pneubeck wrote:
> isn't in this case the NetworkState broken and will never be "repaired"?
> can we ensure that such NetworkState is destructed/removed immediately? if not
> and NetworkStateHandler still calls set_update_received(), we might even use
> such incomplete states.
> 
> Alternatively, instead of checking (e.g. in GetNetworkState) for
> update_received, we could use a new function
>   bool NetworkState::IsValid() {
>     return update_received() && !type_.empty();
>   }

I put these checks in everywhere to track down the actual culprit, but yes, this
would be a major bug. It shouldn't ever happen, I don't think we need to add
special handling (certainly not in this CL), but if it ever did happen I thought
it would be better to log a specific error rather than get an error in
shill_property_util::GetNameFromProperties.

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/shill_p...
File chromeos/network/shill_property_util.cc (right):

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/shill_p...
chromeos/network/shill_property_util.cc:128: if
(properties.GetString(shill::kGuidProperty, &result))
On 2014/03/21 09:33:42, pneubeck wrote:
> ...WithoutPathExpansion...
> (I hate that verbosity) :-/
Yeah, so do I. Strictly speaking it's not necessary here, but I think we did
decide to always use that with Shill since sometime's there's a '.' in there
where we don't expect one. Done.

[pneubeck (no reviews), 2014-03-21 19:52:18]

lgtm

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/shill_p...
File chromeos/network/shill_property_util.cc (right):

https://codereview.chromium.org/207183003/diff/20001/chromeos/network/shill_p...
chromeos/network/shill_property_util.cc:128: if
(properties.GetString(shill::kGuidProperty, &result))
On 2014/03/21 17:08:51, stevenjb wrote:
> On 2014/03/21 09:33:42, pneubeck wrote:
> > ...WithoutPathExpansion...
> > (I hate that verbosity) :-/
> Yeah, so do I. Strictly speaking it's not necessary here, but I think we did
> decide to always use that with Shill since sometime's there's a '.' in there
> where we don't expect one. Done.
> 

Yes, and the method names are simply wrong: without path expansion should be the
default, as it's also the function that does less / is more efficient.
The other should be 'WithPathExpansion'...

[stevenjb, 2014-03-23 20:43:49]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-23 20:43:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/207183003/40001

[commit-bot: I haz the power, 2014-03-23 21:35:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-23 21:35:35]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[stevenjb, 2014-03-24 16:57:30]

Committed patchset #4 manually as r258956 (presubmit successful).