shill: Added message-specific callbacks to the nl80211 code. [chromiumos/platform/shill : master]

4 views
Skip to first unread message

Wade Guthrie (Code Review)

unread,
Oct 30, 2012, 12:00:53 PM10/30/12
to Wade Guthrie
Wade Guthrie has uploaded a new change for review.

Change subject: shill: Added message-specific callbacks to the nl80211 code.
......................................................................

shill: Added message-specific callbacks to the nl80211 code.

This allows users to create a one-off nl80211 message, install a
callback for that message, and then send the message to the kernel.
When the response is issued, the callback will be called and then
removed from the system.

BUG=chromium-os:35129
TEST=Manual and unit tests (including new ones).

Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
---
M Makefile
M callback80211_object.cc
M callback80211_object.h
M config80211.cc
M config80211.h
M config80211_unittest.cc
M kernel_bound_nlmessage.cc
M kernel_bound_nlmessage.h
A mock_callback80211_object.cc
M mock_callback80211_object.h
M mock_nl80211_socket.h
M netlink_socket.cc
M netlink_socket.h
M nl80211_socket.cc
M nl80211_socket.h
M user_bound_nlmessage.cc
M user_bound_nlmessage.h
17 files changed, 484 insertions(+), 258 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/platform/shill refs/changes/04/36904/1
--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

Ben Chan (Code Review)

unread,
Nov 1, 2012, 4:12:19 PM11/1/12
to Wade Guthrie, Gerrit, Paul Stewart, Christopher Wiley
Ben Chan has posted comments on this change.

Change subject: shill: Added message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 1: (4 inline comments)

....................................................
File netlink_socket.cc
Line 128:
nit: remove this empty line

Line 198: number = 1;
Is it safe to assume the seqno to be 1?

....................................................
File nl80211_socket.h
Line 57: // "mlme".
nit: describe |id|

....................................................
File user_bound_nlmessage.h
Line 425: static const uint8_t kCommand;
can these be private as you already provide getters?

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>
Gerrit-Reviewer: Ben Chan <ben...@chromium.org>
Gerrit-Reviewer: Christopher Wiley <wi...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Paul Stewart <ps...@chromium.org>

Wade Guthrie (Code Review)

unread,
Nov 2, 2012, 2:20:59 PM11/2/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Christopher Wiley
Wade Guthrie has posted comments on this change.

Change subject: shill: Added message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 2:

OK, Wiley has a point -- the original CL had more than it needed to. Patch set #2 is _only_ the message-specific callbacks and the unittests therefor. I'll upload the stuff that actually uses the callbacks in a later CL.

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>
Gerrit-Reviewer: Ben Chan <ben...@chromium.org>
Gerrit-Reviewer: Christopher Wiley <wi...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Paul Stewart <ps...@chromium.org>
Gerrit-Reviewer: Wade Guthrie <w...@chromium.org>
Gerrit-Reviewer: Wade Guthrie <w...@google.com>

Wade Guthrie (Code Review)

unread,
Nov 2, 2012, 2:21:48 PM11/2/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Christopher Wiley
Wade Guthrie has posted comments on this change.

Change subject: shill: Added message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 1: (4 inline comments)

So, I've fixed a number of these things in my local repository but I've broken this CL into multiple pieces and all of this is in the piece I haven't yet uploaded.

....................................................
File netlink_socket.cc
Line 128:
Done

Line 198: number = 1;
I believe so, yes. The kernel should just use this number in the response to the message. The only place that would matter is if someone tried to install a callback for this message and there was already a message#1 -- then the user would have to call 'GetSequenceNumber' again. To the best of my knowledge, there's really no issue with sequence numbers being out of sequence.

....................................................
File nl80211_socket.h
Line 57: // "mlme".
Done

....................................................
File user_bound_nlmessage.h
Line 425: static const uint8_t kCommand;
The 'static' members are used by a factory in order to determine the type of the object to make. Given that, an accessor wouldn't work.

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

Christopher Wiley (Code Review)

unread,
Nov 5, 2012, 7:14:24 PM11/5/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Wade Guthrie
Christopher Wiley has posted comments on this change.

Change subject: shill: Added message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 2: (3 inline comments)

....................................................
File config80211.h
Line 201: std::map<uint32_t, Callback> message_callback_;
s/message_callback_/message_callbacks_

Why is this a naked type while we have typedefs for EventTypeStrings and SubscribedEvents?

....................................................
File config80211_unittest.cc
Line 459: TEST_F(Config80211Test, CallbackTest) {
s/CallbackTest/BroadcastCallbackTest

....................................................
File mock_nl80211_socket.h
Line 45:
I would much rather you tied this state into the mock socket instance or the unit test instance rather than creating this static singleton.

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

Wade Guthrie (Code Review)

unread,
Nov 5, 2012, 8:21:23 PM11/5/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Christopher Wiley, Wade Guthrie
Wade Guthrie has posted comments on this change.

Change subject: shill: Added message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 2: (3 inline comments)

Thanks. PTAL.

....................................................
File config80211.h
Line 201: std::map<uint32_t, Callback> message_callback_;
Done

....................................................
File config80211_unittest.cc
Line 459: TEST_F(Config80211Test, CallbackTest) {
Done

....................................................
File mock_nl80211_socket.h
Line 45:
Done

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

mukesh agrawal (Code Review)

unread,
Nov 5, 2012, 8:27:13 PM11/5/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Christopher Wiley, Wade Guthrie
mukesh agrawal has posted comments on this change.

Change subject: shill: Added message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 3: (1 inline comment)

....................................................
Commit Message
Line 7: shill: Added message-specific callbacks to the nl80211 code.
Added -> Add ?

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>
Gerrit-Reviewer: Ben Chan <ben...@chromium.org>
Gerrit-Reviewer: Christopher Wiley <wi...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Paul Stewart <ps...@chromium.org>
Gerrit-Reviewer: Wade Guthrie <w...@chromium.org>
Gerrit-Reviewer: Wade Guthrie <w...@google.com>
Gerrit-Reviewer: mukesh agrawal <qui...@chromium.org>

Christopher Wiley (Code Review)

unread,
Nov 5, 2012, 9:43:37 PM11/5/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Wade Guthrie, mukesh agrawal
Christopher Wiley has posted comments on this change.

Change subject: shill: Added message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 3: (2 inline comments)

....................................................
File callback80211_object.h
Line 58: Config80211 *config80211_;
Not asking for a change, but why are these protected anyway?

....................................................
File mock_callback80211_object.h
Line 20: const base::Callback<void(const UserBoundNlMessage &)> &GetCallback();
I'm sorry I didn't catch this the first time.

Please use Config80211::Callback as your return type.

Could this be made inline?

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

Wade Guthrie (Code Review)

unread,
Nov 6, 2012, 9:42:47 AM11/6/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Christopher Wiley, Wade Guthrie, mukesh agrawal
Wade Guthrie has posted comments on this change.

Change subject: shill: Add message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 3: (3 inline comments)

Thanks, all. PTAL.

....................................................
File callback80211_object.h
Line 58: Config80211 *config80211_;
Because I use this as a base class and child classes often need callback_ and config80211_.

....................................................
Commit Message
Line 7: shill: Added message-specific callbacks to the nl80211 code.
Done

....................................................
File mock_callback80211_object.h
Line 20: const base::Callback<void(const UserBoundNlMessage &)> &GetCallback();
Done

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

Christopher Wiley (Code Review)

unread,
Nov 6, 2012, 11:41:01 AM11/6/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Wade Guthrie, mukesh agrawal
Christopher Wiley has posted comments on this change.

Change subject: shill: Add message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 4: Looks good to me, but someone else must approve

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

Paul Stewart (Code Review)

unread,
Nov 6, 2012, 11:54:30 AM11/6/12
to Wade Guthrie, Gerrit, Ben Chan, Christopher Wiley, Wade Guthrie, mukesh agrawal
Paul Stewart has posted comments on this change.

Change subject: shill: Add message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 4: (1 inline comment)

....................................................
File callback80211_object.h
Line 58: Config80211 *config80211_;
I missed the earlier discussion of why this needed to be protected, and more interestingly why it needs to be interspersed with a "private" section.

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

Christopher Wiley (Code Review)

unread,
Nov 6, 2012, 12:14:26 PM11/6/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Wade Guthrie, mukesh agrawal
Christopher Wiley has posted comments on this change.

Change subject: shill: Add message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 4: (1 inline comment)

....................................................
File callback80211_object.h
Line 58: Config80211 *config80211_;
The interspersed part is because the callback depends on the weak pointer factory and needs the factory to be initialized first in the constructor.

The protected part seems to be because other callbacks are expected to subclass this class.

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

Christopher Wiley (Code Review)

unread,
Nov 6, 2012, 12:14:39 PM11/6/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Wade Guthrie, mukesh agrawal
Christopher Wiley has posted comments on this change.

Change subject: shill: Add message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 4: Looks good to me, approved

Talked with Paul. Good to go!

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>

Wade Guthrie (Code Review)

unread,
Nov 6, 2012, 12:30:08 PM11/6/12
to Wade Guthrie, Gerrit, Ben Chan, Paul Stewart, Christopher Wiley, Wade Guthrie, mukesh agrawal
Wade Guthrie has posted comments on this change.

Change subject: shill: Add message-specific callbacks to the nl80211 code.
......................................................................


Patch Set 4: Verified; Ready

w00t!

--
To view, visit https://gerrit.chromium.org/gerrit/36904
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06bf8d16629f3eac226209b49827289e7a9cdca3
Gerrit-PatchSet: 4
Gerrit-Project: chromiumos/platform/shill
Gerrit-Branch: master
Gerrit-Owner: Wade Guthrie <w...@chromium.org>
Reply all
Reply to author
Forward
0 new messages