Feature/add support for nl80211 standard commands to BWL #900

mariomaz · 2020-02-27T18:46:21Z

Create C++ wrappers around libnl and libnl-genl to facilitate sending
NL80211 commands through a NL80211 netlink socket.

Classes have been created in bwl/shared so NL80211 standard commands
will be available to all flavours of the BWL library except the Dummy one.

Add NL80211_CMD_GET_STATION command implementation as an example of
how standard commands are to be implemented.

Once this design had been validated, the rest of standard commands will
be added. Also we will do a code refactoring to use the new classes and
eliminate duplicated code.

common/beerocks/bwl/shared/netlink_socket.cpp

rmelotte

Everything is documented ✔️
Dynamic allocation uses smart pointers ✔️
It looks excellent!

I have a few comments you might want to read though so I'm approving, but also adding the "don't merge" tag to prevent Mergify from merging it automatically in cases someone else approves it in the meantime.

Overall I'm also not a big fan of having everything in a single commit, but I don't think it's a blocker.

common/beerocks/bwl/shared/netlink_socket.cpp

common/beerocks/bwl/shared/nl80211_socket.cpp

common/beerocks/bwl/shared/nl80211_client_impl.cpp

common/beerocks/bwl/shared/nl80211_client_factory.cpp

rmelotte · 2020-03-02T10:35:58Z

common/beerocks/bwl/dummy/nl80211_client_factory_dummy.cpp

+std::unique_ptr<nl80211_client> nl80211_client_factory::create_instance()
+{
+    return std::make_unique<nl80211_client_dummy>();
+}


This factory doesn't look very useful... I guess it's there for consistency with other implementations?

As said, this is the implementation of the factory in the Dummy flavor of the BWL library. The factory is defined to return an instance of a class that implements the interface nl80211_client. This particular implementation returns an instance of nl80211_client_dummy. Methods of this class do nothing but return dummy data.

(nitpick) the rest of the bwl uses a factory function instead of a factory class, e.g. ap_wlan_hal_create(). It would be more consistent to use a factory function nl80211_client_create() here as well.

edjaguilar · 2020-03-02T11:30:26Z

@rmelotte asked me to review this change as well.
I just have one comment to add, I changed the CMakeLists to search dynamically for directories with the structure of the DUMMY bwl, based on strings in the path. If I merge this change to the econet bwl later I might remove the separate case for DUMMY bwl.

tomereli

Very good job documenting everything.
I think this should have been split up to more commits, but I don't think its worth it at this point.
I don't really like the fact we need to treat BWL DUMMY mode differently in the CMAKE. The fact that we implement this in shared leads to code duplication (that you probably mean to remove in future PR), but I think it should be handled as part of this PR.

The way I would do it, is by introducing another level of inheritance to the DWPAL implementation like @vitalybu suggested when we discussed this - have BWL DWPAL derive from BWL nl80211, and override everything except for the nl80211 standard commands.
The BWL Dummy should just implement stubs like you did in this PR.
I mark this as needs changes so @arnout and @vitalybu can comment on the direction in which this is going.

common/beerocks/bwl/include/bwl/nl80211_client.h

common/beerocks/bwl/shared/netlink_socket.cpp

common/beerocks/bwl/shared/nl80211_client_factory.cpp

vitalybu

Overall looks good, but I do have some comments, so not approving just yet

common/beerocks/bwl/CMakeLists.txt

common/beerocks/bwl/dummy/nl80211_client_dummy.cpp

common/beerocks/bwl/include/bwl/nl80211_client.h

common/beerocks/bwl/shared/netlink_socket.cpp

mariomaz · 2020-03-05T18:11:21Z

Typo - DWPAL

Fixed

Missing line break

Fixed

Can m_socket be nullptr here?

Fixed

mariomaz · 2020-03-05T18:42:28Z

Very good job documenting everything.
I think this should have been split up to more commits, but I don't think its worth it at this point.
I don't really like the fact we need to treat BWL DUMMY mode differently in the CMAKE. The fact that we implement this in shared leads to code duplication (that you probably mean to remove in future PR), but I think it should be handled as part of this PR.

The way I would do it, is by introducing another level of inheritance to the DWPAL implementation like @vitalybu suggested when we discussed this - have BWL DWPAL derive from BWL nl80211, and override everything except for the nl80211 standard commands.
The BWL Dummy should just implement stubs like you did in this PR.
I mark this as needs changes so @arnout and @vitalybu can comment on the direction in which this is going.

I evaluated @vitalybu's suggestion but decided to go with this solution because it's much simpler. Also, if BWL DWPAL derives from BWL nl80211, then it will take longer to compile and the binary will be bigger.

BWL DWPAL, BWL nl80211 and whatever other part of the application can all be users of the nl80211_client_factory, create an instance of a class implementing the nl80211_client interface and use it to send NL80211 standard commands. Although I prefer to inject the dependencies to facilitate unit-testing (if we ever had to), so most probably BWL DWPAL and BWL nl80211 classes that send NL80211 standard commands will have a nl80211_client parameter in their constructor.

Another goal of this design is that it can be easily extended to send NETLINK_ROUTE commands (probably with a netlink_route_client). Maybe we should move all these classes from BWL to BPL because they are not intended for WiFi only.

Define a structure to hold collected link metrics. This structure will later be used to fill the TLVs of the Link Metric response message. Use uint32_t instead of uint16_t for max throughput capacity and bit rate fields to allow for bigger values. Spec document uses uint16_t so a saturation to the maximum value is required when copying values to the TLV. Link metrics have to be collected differently for wired and wireless links. Define an interface class for a link metrics collector. Derived implementation classes will override pure virtual methods. Create an implementation class for Ethernet links. This implementation reads network interface stats using a Netlink socket and the NETLINK_ROUTE protocol. TODO in future tasks: - Implement link metrics collector for WiFi links in ([TASK] Link metric collection for WiFi links #792) - Move all link metrics related classes to BPL ([TASK] Move link metric related classes to BPL #910) - Implement Ethernet link metrics collector using and extending Netlink classes defined in "Feature/add support for nl80211 standard commands to BWL #900" Signed-off-by: Mario Maz <[email protected]>

arnout · 2020-03-06T10:43:47Z

The way I would do it, is by introducing another level of inheritance to the DWPAL implementation like @vitalybu suggested when we discussed this - have BWL DWPAL derive from BWL nl80211, and override everything except for the nl80211 standard commands.

Note that this looks like implementation inheritance, and 25 years ago I was thought that this is evil.

The BWL Dummy should just implement stubs like you did in this PR.
I mark this as needs changes so @arnout and @vitalybu can comment on the direction in which this is going.

I evaluated @vitalybu's suggestion but decided to go with this solution because it's much simpler. Also, if BWL DWPAL derives from BWL nl80211, then it will take longer to compile and the binary will be bigger.

BWL DWPAL, BWL nl80211 and whatever other part of the application can all be users of the nl80211_client_factory, create an instance of a class implementing the nl80211_client interface and use it to send NL80211 standard commands. Although I prefer to inject the dependencies to facilitate unit-testing (if we ever had to), so most probably BWL DWPAL and BWL nl80211 classes that send NL80211 standard commands will have a nl80211_client parameter in their constructor.

Another goal of this design is that it can be easily extended to send NETLINK_ROUTE commands (probably with a netlink_route_client). Maybe we should move all these classes from BWL to BPL because they are not intended for WiFi only.

Please include this reasoning (more or less copy&paste of the text above) in one of the commit messages.

tomereli

A couple of nitpicks but strong LGTM!

tomereli · 2020-03-08T10:49:34Z

common/beerocks/bwl/shared/README.md

@@ -0,0 +1,6 @@
+# Shared files directory
+
+The `shared` directory contains classes shared among all the implementations that use actual WiFi radios (i.e.: the BWPAL and NL80211 flavors but not the Dummy one).


Might be worth adding documentation to the README.md for the nl80211 support which is added in this PR.
It might be useful to read through the README.md which will serve as the DR for the implementation before browsing the code.
Anyway, this is one hell of a nitpick, so ignore it if you like :)

common/beerocks/bwl/shared/netlink_socket.cpp

arnout

Excellent documentation in this PR. I wish all the code was like that.

As often, I have a truckload of comments, but nothing critical. Except for one thing: the negative error number. It would have been great if there were a unit test that would validate such issues :-)

Given that there is nothing critical, I'm approving, so please do the following:

apply the fixups that you agree with and are easy to do;
for bigger changes that you think really should be done, make an issue;
for the hints of the type "code could be refactored like this", just leave them as unresolved - we can refer back to this PR when development on the netlink stuff continues;
squash and rebase everything, and mergify!

arnout · 2020-03-09T10:18:33Z

common/beerocks/bwl/dummy/nl80211_client_factory_dummy.cpp

+std::unique_ptr<nl80211_client> nl80211_client_factory::create_instance()
+{
+    return std::make_unique<nl80211_client_dummy>();
+}


(nitpick) the rest of the bwl uses a factory function instead of a factory class, e.g. ap_wlan_hal_create(). It would be more consistent to use a factory function nl80211_client_create() here as well.

arnout · 2020-03-09T10:19:59Z

common/beerocks/bwl/include/bwl/nl80211_client.h

+ * This interface lists the methods to read and write wireless hardware status and configuration
+ * through a NL80211 socket.
+ *
+ * Programming to an interface allows dependent classes to remain unaware of the different
+ * implementations of the interface as well as facilitates the creation of mock implementations for
+ * unit testing.
+ *
+ * This is a C++ interface: an abstract class that is designed to be specifically used as a base
+ * class and which derived classes (implementations) will override each pure virtual function.
+ *
+ * Known implementations: nl80211_client_impl (uses NL80211 socket), nl80211_client_dummy (fake
+ * implementation that returns dummy data).


Very good documentation, thank you!

common/beerocks/bwl/include/bwl/nl80211_client.h

arnout · 2020-03-09T12:23:49Z

common/beerocks/bwl/shared/netlink_socket.cpp

+bool netlink_socket::connect()
+{
+    // Connect the socket
+    if (nl_connect(m_nl_socket.get(), m_protocol) != 0) {


(For my understanding, not a change request:) Why is the explicit get() needed here? Is it because nl_connect is an extern C that there's no automatic conversion?

Honestly, I didn't know it was even possible to perform such implicit conversion from smart pointer to raw pointer type. But even if it was possible, in this case I'd rather use the explicit access.

If I remove the call to get(), then I get this error when compiling for the RAX40:

../common/beerocks/bwl/shared/netlink_socket.cpp: In member function 'virtual bool bwl::netlink_socket::connect()': ../common/beerocks/bwl/shared/netlink_socket.cpp:29:43: error: cannot convert 'std::unique_ptr<nl_sock, void (*)(nl_sock*)>' to 'nl_sock*' for argument '1' to 'int nl_connect(nl_sock*, int)' if (nl_connect(m_nl_socket, m_protocol) != 0) { ^

I found these quite interesting articles about the topic:

7.7 Implicit Conversion to Raw Pointer Types

Reader Q&A: Why don’t modern smart pointers implicitly convert to *?

arnout · 2020-03-09T13:51:39Z

common/beerocks/bwl/shared/nl80211_socket.cpp

+#include <easylogging++.h>
+
+/**
+ * Receive and transmit socket buffer size in bytes.


Could be useful to mention why this is changed from the default.

Honestly, I don't know. I've been investigating and found that the call to nl_socket_set_buffer_size ends up calling setsockopt(SO_SNDBUF) and setsockopt(SO_RCVBUF). Some people report that they were losing events with default values and had to increase the receive buffer size to fix it. Some tutorials I've found also change default sizes but don't explain why.

I borrowed this code from base_wlan_hal_nl80211::send_nl80211_msg and thought that who initially wrote that code had considered it was necessary to call nl_socket_set_buffer_size so I kept it. I didn't question myself why it was actually needed nor if values used were the most appropriate.

While investigating, I've realized that the call to nl_socket_set_buffer_size requires the socket to be connected or otherwise it fails with NLE_BAD_SOCK. Thus I've removed the call from the constructor and put it after the socket connection has been made.

base_wlan_hal_nl80211 is also calling nl_socket_set_buffer_size but before connecting the socket so I guess it must be failing too (no error code is being checked after the call) and buffer size is not being changed. Thus it looks like, for the commands being executed up to now in BWL, it's not necessary to increase buffer size.

arnout · 2020-03-09T13:53:36Z

common/beerocks/bwl/shared/nl80211_socket.cpp

+        const char *family_name = "nl80211";
+
+        m_family_id = genl_ctrl_resolve(m_nl_socket.get(), family_name);
+        if (0 > m_family_id) {
+            LOG(ERROR) << "'" << family_name << "' family not found!";
+            result = false;


So in my proposal, this bit would go into post_connect() and the error handling (i.e. calling close()) would go into the non-virtual connect() function.

common/beerocks/bwl/shared/nl80211_socket.h

common/beerocks/bwl/shared/nl_genl_socket.cpp

arnout · 2020-03-09T14:06:13Z

common/beerocks/bwl/shared/nl_genl_socket.h

+/**
+ * @brief C++ wrapper class around the generic Netlink protocol library (libnl-genl).
+ *
+ * This is a very simple C++ wrapper class around the generic Netlink protocol library. Its aim is
+ * to facilitate the use of the library from C++ applications. This class extends the
+ * netlink_socket base class for the generic Netlink protocol (NETLINK_GENERIC).
+ *
+ * Known derived classes: nl80211_socket.
+ */


With no functionality other than setting the protocol, I'm not sure if it's worthwhile to introduce this additional class. IMO it makes the code harder to understand with no benefit at all.

This class is only useful if later on

there is a second, non-nl80211 netlink socket (and there probably should be one, for nlrt, but that one will not be in bwl so code needs to be refactored anyway);

and there is some common code between them that should be in this class and not in netlink_socket itself.

But in that case, we can just introduce the class at that time.

We need support for nl80211 standard commands for things such as obtaining link metrics or device capabilities. In "Send standard NL80211 commands using DWPAL #782" we firstly evaluated the convenience of adding such support in DWPAL and then in BWL. As a result of such evaluation we concluded that DWPAL was not the most suitable place to add such support. Reason is that NL80211 standard commands are not exclusive of devices where DWPAL is used and support is also required for other flavors of the BWL library. Then we decided to add support for standard commands into the BWL library, but shared to all flavors, not only for the DWPAL version. A proposal was made to include support for NL80211 standard commands in the NL80211 classes, then derive BWL DWPAL from BWL NL80211 and override everything except the standard commands. This proposal was not finally implemented to "favor composition instead of inheritance". Instead of inheritance, a nl8011_client interface and a factory for objects of classes implementing such interface have been defined. BWL DWPAL, BWL nl80211 and whatever other part of the application can all be users of the nl80211_client factory, create an instance of a class implementing the nl80211_client interface and use it to send NL80211 standard commands. Although I prefer to inject the dependencies to facilitate unit-testing (if we ever had to), so most probably BWL DWPAL and BWL nl80211 classes that send NL80211 standard commands will have a nl80211_client parameter in their constructor instead of calling the factory. Another goal of this design is that it can be easily extended to send NETLINK_ROUTE commands (probably with a netlink_route_client). Maybe we should move all these classes from BWL to BPL because they are not intended for WiFi only. Created C++ wrappers around libnl and libnl-genl to facilitate sending NL80211 commands through a NL80211 netlink socket. Classes have been created in bwl/shared so NL80211 standard commands will be available to all flavours of the BWL library (except the Dummy one). Public include files have been created in bwl/include. Created a C++ interface with the list of commands available. Create two implementations, one to actually send those commands to the WiFi driver and a fake one for the Dummy implementation of the BWL library. Created a factory class in each BWL implementation to create instances of the NL80211 client. This factory will be used both from within the BWL library or from outside. Add NL80211_CMD_GET_STATION command implementation as an example of how standard commands are to be implemented. Once this design had been validated, the rest of standard commands will be added. Also we will do a code refactoring to use the new classes and eliminate duplicated code. Signed-off-by: Mario Maz <[email protected]>

Define a structure to hold collected link metrics. This structure will later be used to fill the TLVs of the Link Metric response message. Use uint32_t instead of uint16_t for max throughput capacity and bit rate fields to allow for bigger values. Spec document uses uint16_t so a saturation to the maximum value is required when copying values to the TLV. Link metrics have to be collected differently for wired and wireless links. Define an interface class for a link metrics collector. Derived implementation classes will override pure virtual methods. Create an implementation class for Ethernet links. This implementation reads network interface stats using a Netlink socket and the NETLINK_ROUTE protocol. TODO in future tasks: - Implement link metrics collector for WiFi links in ([TASK] Link metric collection for WiFi links #792) - Move all link metrics related classes to BPL ([TASK] Move link metric related classes to BPL #910) - Implement Ethernet link metrics collector using and extending Netlink classes defined in "Feature/add support for nl80211 standard commands to BWL #900" Signed-off-by: Mario Maz <[email protected]>

mariomaz requested review from arnout, morantr, rmelotte and tomereli as code owners February 27, 2020 18:46

mariomaz self-assigned this Feb 27, 2020

mariomaz added this to the M1 - Certifiable agent with wired backhaul milestone Feb 27, 2020

mariomaz force-pushed the feature/add-nl80211-standard-commands-support branch 4 times, most recently from 18e14ae to 53dbca9 Compare February 28, 2020 15:30

ghost reviewed Feb 28, 2020

View reviewed changes

common/beerocks/bwl/shared/netlink_socket.cpp Outdated Show resolved Hide resolved

mariomaz mentioned this pull request Mar 2, 2020

Send standard NL80211 commands using DWPAL #782

Closed

rmelotte approved these changes Mar 2, 2020

View reviewed changes

rmelotte requested a review from edjaguilar March 2, 2020 11:00

ghost requested a review from vitalii-komisarenko March 2, 2020 11:05

rmelotte added the don't merge This PR is not ready for merge or review label Mar 2, 2020

mariomaz mentioned this pull request Mar 2, 2020

Feature/collect link metrics #831

Merged

tomereli requested a review from vitalybu March 2, 2020 17:07

tomereli suggested changes Mar 2, 2020

View reviewed changes

vitalii-komisarenko reviewed Mar 3, 2020

View reviewed changes

common/beerocks/bwl/include/bwl/nl80211_client.h Show resolved Hide resolved

common/beerocks/bwl/shared/netlink_socket.cpp Show resolved Hide resolved

common/beerocks/bwl/shared/nl80211_client_factory.cpp Show resolved Hide resolved

vitalybu reviewed Mar 4, 2020

View reviewed changes

mariomaz requested review from vitalybu, tomereli, rmelotte and a user March 6, 2020 08:27

mariomaz requested a review from vitalii-komisarenko March 6, 2020 08:27

tomereli approved these changes Mar 8, 2020

View reviewed changes

arnout approved these changes Mar 9, 2020

View reviewed changes

mariomaz force-pushed the feature/add-nl80211-standard-commands-support branch from 03a8a7e to 7f93d9f Compare March 11, 2020 13:33

mariomaz removed the don't merge This PR is not ready for merge or review label Mar 11, 2020

tomereli mentioned this pull request Mar 11, 2020

Update the controller on son_slave connect/disconnect event #931

Merged

arnout force-pushed the feature/add-nl80211-standard-commands-support branch from 7f93d9f to 40afb37 Compare March 11, 2020 15:51

mergify bot merged commit b3c9315 into master Mar 11, 2020

mergify bot deleted the feature/add-nl80211-standard-commands-support branch March 11, 2020 18:13

mariomaz mentioned this pull request Mar 26, 2020

[TASK] Move Netlink related classes to BPL #1027

Closed

mariomaz mentioned this pull request Apr 21, 2020

[TASK] Link metric collection for WiFi links #792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add support for nl80211 standard commands to BWL #900

Feature/add support for nl80211 standard commands to BWL #900

mariomaz commented Feb 27, 2020 •

edited

Loading

rmelotte left a comment

rmelotte Mar 2, 2020

mariomaz Mar 5, 2020

arnout Mar 9, 2020

edjaguilar commented Mar 2, 2020

tomereli left a comment

vitalybu left a comment

mariomaz commented Mar 5, 2020 •

edited

Loading

mariomaz commented Mar 5, 2020

arnout commented Mar 6, 2020

tomereli left a comment

tomereli Mar 8, 2020

arnout left a comment

arnout Mar 9, 2020

arnout Mar 9, 2020

arnout Mar 9, 2020

mariomaz Mar 10, 2020

arnout Mar 9, 2020

mariomaz Mar 11, 2020

arnout Mar 9, 2020

arnout Mar 9, 2020

		@@ -0,0 +1,6 @@
		# Shared files directory

		The `shared` directory contains classes shared among all the implementations that use actual WiFi radios (i.e.: the BWPAL and NL80211 flavors but not the Dummy one).

Feature/add support for nl80211 standard commands to BWL #900

Feature/add support for nl80211 standard commands to BWL #900

Conversation

mariomaz commented Feb 27, 2020 • edited Loading

rmelotte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edjaguilar commented Mar 2, 2020

tomereli left a comment

Choose a reason for hiding this comment

vitalybu left a comment

Choose a reason for hiding this comment

mariomaz commented Mar 5, 2020 • edited Loading

mariomaz commented Mar 5, 2020

arnout commented Mar 6, 2020

tomereli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariomaz commented Feb 27, 2020 •

edited

Loading

mariomaz commented Mar 5, 2020 •

edited

Loading