Add path open errors from the permission broker to the device log.

Add path open errors from the permission broker to the device log.

This change adds an additional ErrorCallback parameter to the permission
broker client so that the caller can get a better log message when its
request to open a device node fails.

This will hopefully help to determine the cause of the many "Did not get
valid device handle from permission broker" errors that some users are
seeing.

BUG=570784
Committed: https://crrev.com/ef3511f30c78a2e286c7dc427b2af18ec54cb0c9
Cr-Commit-Position: refs/heads/master@{#374571}

[Reilly Grant (use Gerrit), 2016-02-09 22:25:52]

reillyg@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Reilly Grant (use Gerrit), 2016-02-09 22:25:52]

Please take a look. I'm embarrassed by the utter uselessness of the original
error message.

[stevenjb, 2016-02-09 23:16:51]

lgtm

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
File chromeos/dbus/fake_permission_broker_client.cc (right):

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
chromeos/dbus/fake_permission_broker_client.cc:38: base::Bind(error_callback,
"open_failed",
nit: define 'open_failed' as a const, make available in header

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
chromeos/dbus/fake_permission_broker_client.cc:41:
logging::SystemErrorCodeToString(error_code).c_str())));
nit: return here, remove else

[Reilly Grant (use Gerrit), 2016-02-09 23:54:03]

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
File chromeos/dbus/fake_permission_broker_client.cc (right):

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
chromeos/dbus/fake_permission_broker_client.cc:38: base::Bind(error_callback,
"open_failed",
On 2016/02/09 23:16:51, stevenjb wrote:
> nit: define 'open_failed' as a const, make available in header

Which header do you recommend? permission_broker_client.h or
service_constants.h? Ideally these errors would be part of the interface exposed
by the permission broker service.

[stevenjb, 2016-02-10 00:01:02]

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
File chromeos/dbus/fake_permission_broker_client.cc (right):

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
chromeos/dbus/fake_permission_broker_client.cc:38: base::Bind(error_callback,
"open_failed",
On 2016/02/09 23:54:03, Reilly Grant wrote:
> On 2016/02/09 23:16:51, stevenjb wrote:
> > nit: define 'open_failed' as a const, make available in header
> 
> Which header do you recommend? permission_broker_client.h or
> service_constants.h? Ideally these errors would be part of the interface
exposed
> by the permission broker service.

Oh, I didn't notice this was the fake. In that case feel free to ignore this,
otherwise we should use the same const that the real implementation uses if
available, or define that as a const in this file.

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/permission_br...
File chromeos/dbus/permission_broker_client.cc (right):

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/permission_br...
chromeos/dbus/permission_broker_client.cc:153: error_name =
"org.chromium.Error.NoResponse";
nit: define at the top of the file:

const char kNoResponseError[] = "org.chromium.Error.NoResponse";

[Reilly Grant (use Gerrit), 2016-02-10 00:16:32]

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
File chromeos/dbus/fake_permission_broker_client.cc (right):

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
chromeos/dbus/fake_permission_broker_client.cc:38: base::Bind(error_callback,
"open_failed",
On 2016/02/10 00:01:01, stevenjb wrote:
> On 2016/02/09 23:54:03, Reilly Grant wrote:
> > On 2016/02/09 23:16:51, stevenjb wrote:
> > > nit: define 'open_failed' as a const, make available in header
> > 
> > Which header do you recommend? permission_broker_client.h or
> > service_constants.h? Ideally these errors would be part of the interface
> exposed
> > by the permission broker service.
> 
> Oh, I didn't notice this was the fake. In that case feel free to ignore this,
> otherwise we should use the same const that the real implementation uses if
> available, or define that as a const in this file.

Done.

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/fake_permissi...
chromeos/dbus/fake_permission_broker_client.cc:41:
logging::SystemErrorCodeToString(error_code).c_str())));
On 2016/02/09 23:16:51, stevenjb wrote:
> nit: return here, remove else

Done.

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/permission_br...
File chromeos/dbus/permission_broker_client.cc (right):

https://codereview.chromium.org/1681383002/diff/1/chromeos/dbus/permission_br...
chromeos/dbus/permission_broker_client.cc:153: error_name =
"org.chromium.Error.NoResponse";
On 2016/02/10 00:01:02, stevenjb wrote:
> nit: define at the top of the file:
> 
> const char kNoResponseError[] = "org.chromium.Error.NoResponse"; 

Done.

[Reilly Grant (use Gerrit), 2016-02-10 00:16:36]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-02-10 00:16:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1681383002/#ps20001
(title: "Addresses stevenjb@'s comments.")

[commit-bot: I haz the power, 2016-02-10 00:17:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681383002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681383002/20001

[commit-bot: I haz the power, 2016-02-10 01:53:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-10 01:54:13]

Description was changed from

==========
Add path open errors from the permission broker to the device log.

This change adds an additional ErrorCallback parameter to the permission
broker client so that the caller can get a better log message when its
request to open a device node fails.

This will hopefully help to determine the cause of the many "Did not get
valid device handle from permission broker" errors that some users are
seeing.

BUG=570784
==========

to

==========
Add path open errors from the permission broker to the device log.

This change adds an additional ErrorCallback parameter to the permission
broker client so that the caller can get a better log message when its
request to open a device node fails.

This will hopefully help to determine the cause of the many "Did not get
valid device handle from permission broker" errors that some users are
seeing.

BUG=570784

Committed: https://crrev.com/ef3511f30c78a2e286c7dc427b2af18ec54cb0c9
Cr-Commit-Position: refs/heads/master@{#374571}
==========

[commit-bot: I haz the power, 2016-02-10 01:54:14]

Patchset 2 (id:??) landed as
https://crrev.com/ef3511f30c78a2e286c7dc427b2af18ec54cb0c9
Cr-Commit-Position: refs/heads/master@{#374571}