p4lang / behavioral-model

The reference P4 software switch
Apache License 2.0
550 stars 330 forks source link

Why bmi_port.c is a C code? #1249

Open at-nojavan opened 6 months ago

at-nojavan commented 6 months ago

Hello everyone!

I'm trying to make a backward-compatible edit to the BMv2 simple switch. Simple switch passes a packet handler from the place that static void packet_handler(int port_num, const char *buffer, int len, void *cookie) function is defined here, all the way to the place it's used in the bmi_port.c.

I see that in the dev_mgr_bmi.cpp here, a transform is performed on the handler from std::function to C style function pointer, because the next code in the chain is a C code (bmi_port.c). I am trying to pass a std::function that has capture and this transform is causing some issues for me. Now I have a question. Why bmi_port.c is a C code and not C++. What is the restriction that binds us to use C code? Is it possible to change this to C++?

antoninbas commented 6 months ago

This code is written in C for legacy reasons. It was "copied over" years ago from the previous version of the behavioral model: https://github.com/p4lang/p4factory/tree/master/modules/BMI. There was no strong reason to rewrite this code at the time (although I think it may have been marginally improved over time), as it was working and this is a part of the code that is both essential and hard to test in an automatic way.

That being said, I think rewriting this code in C++ and taking a more modern approach (I would say the select-based approach is probably a bit obsolete now) is a great idea. While I will not work on this, I would be happy to review such a change. I would say that if anyone wants to work on this, please do not start any implementation work until first presenting your proposed approach here and getting some sort of consensus. I would be fine with something that is based on Boost Asio.

Before approving such a change, and because we don't have automated tests for the I/O code, I would expect the following:

fruffy commented 6 months ago

a successful test run of the p4c PTF test suite using the modified bmv2 + veths for packet I/O. Assuming there is such a thing... cc @fruffy

It depends... we do not have many static PTF tests available in the suite, but P4Testgen generates PTF tests for all the BMv2 programs with each CI run. That can be used for testing at least.

github-actions[bot] commented 2 weeks ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment, or this will be closed in 180 days