PPM-249 - independent unit to deal with hostapd configuration

[RanRegev]

Encapsulates hostapd configuration separately.
The separation enables:

• reuse in more than one place: e.g. nl80211, dwpal, mock
• testing
• independent development
  For more detail:
  This PR: https://jira.prplfoundation.org/browse/PPM-249
  General Overview: common/beerocks/hostapd/README.md

[arnout]

None of this is needed. If it turns out to be needed, introduce it when you need it.

[RanRegev]

Removed
Will be introduced when needed.

[arnout]

It's still there.

[mariomaz]

Unit tests should be improved.

[mariomaz]

Should you remove leading and trailing white spaces before checking for empty line?

[mariomaz]

Remove commented code

[mariomaz]

If you use the same file name for all the tests, then you cannot run them in multiple simultaneous threads. It's better to choose a different file name for each test.
To avoid file name conflicts in case more than one instance of the test application was executed in the same system at the same time, I used to use UUIDs for file names.
You should read the TMPDIR environment variable to get the name of the temporary directory instead of assuming it's "/tmp"

[arnout]

To be completely accurate, you should use mkstemp() - that guarantees a unique file.

However, even better would be if the file itself could be mocked away. I tried to convince @RanRegev of this but no success. So, I guess it should be merged as is and @mariomaz can have fun with creating something that uses mocks.

Note that all of Mario's other comments to this unit test would be resolved if the file were mocked away...

[RanRegev]

I was convinced, and will eventually be aligned with this. I have two major issues that hold me from doing it now:

1. Time frame. I need some ramp up to learn and work with these MOCKS.
2. From the little I saw - I would need to have an interface for File just for the sake of the tests. From my point of view, if I write a File interface, it should be an interface that satisfies many kinds of files, including, among others, the mock files. This is entirely different mission than just creating a File interface that is good for my current needs and tests. In other words, thinking only on current needs and mocking will eventually resulted with something that is usable in these two use cases only. This is not an interface as I see it. Interface for me is something that I can use in many places, usually written for one or two existing use cases - but is truly opened for farther extending.

[mariomaz]

Suggested change

	const int value)
	int value)

[mariomaz]

Is this a valid doxygen format for @params?

[arnout]

Nope.

[mariomaz]

Suggested change

	TEST(configuration_test, diable_all_ap)
	TEST(configuration_test, disable_all_ap_vaps)

[mariomaz]

These tests are not actually testing, they are just running the code

[mariomaz]

Shouldn't you set m_ok = false before returning?
Also, if function succeeds, m_last_message should be cleared.
If m_last_message and m_ok are updated in parallel, you might infer m_ok from m_last_message

[arnout]

As usual, I have a lot of comments on this PR.

The commit splitting is a bit of a mess. But I guess I rushed you a bit :-) So OK.

One problem is that it adds a lot of unneeded features. Please don't do that. We don't need commenting out, we don't need changing the header part, we don't need disable of all vaps. So almost half of the commits are not even needed.

That said, nothing is critically wrong. The only critical thing is the ordering of the VAPs that isn't maintained. However, it will work in practice (at least for the time being) since openwrt generates the config file with sorted bss= sections. Well, actually, you can do funny things in uci wireless so that it generates it in a different order. Anyway, I think it can be merged as is (maybe with some cosmetic changes like fixing the errors in doxygen) but a bug has to be created for the ordering of VAPs.

[arnout]

We don't start or stop hostapd.

[arnout]

Suggested change

	/**
	* @brief construct a Configurationn object
	* @param file_name the name of the configuration file.
	*/
	/**
	* @brief Construct a Configuration object.
	*
	* @param file_name the name of the configuration file.
	*/

[arnout]

I like an explicit

Suggested change

	Configuration() = delete;

[arnout]

So, no RAII then?

[arnout]

Suggested change

	// that belongs to the "head" part. read the explnation at
	// that belongs to the "head" part. read the explanation at

[arnout]

Err, so max 10 vaps supported?

[arnout]

I would have turned it around, something like this:

Suggested change

	template <class func, class iter>
	void Configuration::for_all_ap_vaps(func f, iter current_iter, const iter end)
	template <class func, class iter>
	void Configuration::for_all_ap_vaps(func f, iter current_iter, const iter end)
	{
	for_all_ap_vaps([](const std::vector<std::string> &vap) {
	if (current_iter != end) {
	f(vap, current_iter);
	current_iter++;
	} else {
	/* Call the second function here for the remaining vaps */
	}
	});

[arnout]

Ah, actually this way indeed you can handle the remaining ones... But it's clunky because you need the condition inside, a separate function would be nicer.

[arnout]

expect then assert of the same is weird.

[arnout]

Again, we don't need this. We only modify VAPs.

[arnout]

This is wrong. The actual format of the file is:

... Configuration of first VAP
bss=...
... Configuration of second VAP
bss=...
... Configuration of third VAP

interface= sets the name of the VAP. bss= also sets it, so it really is only needed for the first VAP.

As it happens, openwrt generates the configuration file as:

... Global options
interface=first_vap_name
... First VAP options
bss=second_vap_name
... Second VAP options

But that's just "coincidence", not really something we can rely on...

[RanRegev]

Replaced by #1601