[linux/qemu] Qemu bridging v2

[sharder996]

Public side of #647

The spec for this feature can be found here.

This second version of QEMU network bridging was required because of the change in implementation of find_bridge_with(). Since most of bridged networks was already available on macOS/QEMU and implemented, this PR is mostly just for enabling and wiring up the function calls needed to create a network interface on Linux.

The key difference on Linux is the presence of bridge_helper.c. Below is an excerpt from the spec on the purpose of the bridge helper:

Finally, to use bridges on QEMU, on Linux, we will use a bridge helper. The QEMU bridge helper, shipped with QEMU, is a simple program which checks a file in /etc/ to see if using a certain bridge is allowed; then it sets up the devices needed for QEMU to use the bridge. Access to this file would be denied in our snap. Instead, our approach will be to fork the source of the bridge helper and remove the need to check for this file, thus allowing all the bridges we create. Then, from the QEMU command line, we must specify the path to our binary, for avoiding executing the default bridge helper.

[codecov]

Codecov Report

Attention: Patch coverage is 82.35294% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.89%. Comparing base (d596d32) to head (64f0649).
Report is 187 commits behind head on main.

Files	Patch %	Lines
...backends/qemu/linux/qemu_platform_detail_linux.cpp	80.95%	4 Missing ⚠️
...orm/backends/qemu/qemu_virtual_machine_factory.cpp	84.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3573      +/-   ##
==========================================
+ Coverage   88.83%   88.89%   +0.05%     
==========================================
  Files         253      254       +1     
  Lines       14170    14250      +80     
==========================================
+ Hits        12588    12667      +79     
- Misses       1582     1583       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ricab]

Hey @sharder996, thank you for your work on this. I did a quick pass and I have a couple of questions.

[georgeliao]

@sharder996
Since I was not in the discussions, it would be nice to have a context introduction. Something like why bridge_helper executable is needed on linux but not on macos. What was the state of the code before and why did you do some refactoring? Thanks.

[sharder996]

Does clang-format format test_qemu_linux_platform.cpp the same as the linter in CI for anyone else? Don't know why is doesn't format the same for me.

[ricab]

@sharder996 are you perhaps using a different version than CI? Different versions have different ideas of what the format should be.

[georgeliao]

@sharder996
Check the clang-format executable version, check the clang-format configuration file your local IDE uses is the in-repo one by changing the file and see if re-apply formatting makes a difference.

[georgeliao]

@sharder996
It looks very good. I only have a few minor comments. The unit test coverage is not high enough, it is easy to add these tests as well. However, on the other side, the uncovered part does not have any actual logic, so it is ok to skip them. I will leave it to you whether you want to push the test coverage up.

Funcitonal testing was done as well, it works fine on linux and macos.

[georgeliao]

Open for discussion:
we use the hardcoded executable name in CMake and here. It makes more sense to use a variable in CMake and pass it here. The idiom to do this is also simple. Just changing something in the cmake file multipass/src/platform/backends/qemu/linux/CMakeLists.txt.

set(BRIDGE_HELPER_EXEC_NAME "bridge_helper")
add_library(qemu_platform_detail
  dnsmasq_process_spec.cpp
  dnsmasq_server.cpp
  firewall_config.cpp
  qemu_platform_detail_linux.cpp)
target_compile_definitions(qemu_platform_detail PRIVATE BRIDGE_HELPER_EXEC_NAME_CPP="${BRIDGE_HELPER_EXEC_NAME}")

...
and using BRIDGE_HELPER_EXEC_NAME in the below cmake script and use BRIDGE_HELPER_EXEC_NAME_CPP in the c++ code here.

I know that we also have sshfs_server in the pattern. So it is open for discussion whether we want to change them both, or whether we want it to be in a different PR or not. Maybe @ricab also has an opinion on this.

[ricab]

If it is not difficult to do, I would say yes. If you start running into trouble and needing to invest more time for any reason, I would say leave it.

[sharder996]

@georgeliao As for the missing test line coverage. Having no logic to actually test was the reason why I didn't pursue it.

[ricab]

Good stuff, well done!