Skip to content

Netmap: fixed ubuntu compilation error by added strlcpy definition to utility.c#808

Merged
phillip-stephens merged 9 commits intomainfrom
phillip/netmap-ubuntu-compile
Mar 6, 2024
Merged

Netmap: fixed ubuntu compilation error by added strlcpy definition to utility.c#808
phillip-stephens merged 9 commits intomainfrom
phillip/netmap-ubuntu-compile

Conversation

@phillip-stephens
Copy link
Contributor

I was setting up a new Ubuntu VM to test out the Netmap work and was running into compilation errors. Tried a couple of online suggestions (using the lbsd linker flag, for example) to no avail. Instead of trying to figure out the linker/CMake, I just thought I'd take the strlcpy from online (Android source code, to be specific) and add that to utility.c. This works and I've confirmed I'm seeing 60% faster packet send on a standard VM than default ZMap. 🎉

cmake -DWITH_NETMAP=ON -DENABLE_DEVELOPMENT=ON . && make -j4
sudo ./src/zmap -p 80 -B 10G -T 4 -o "/dev/null" -I enp0s9

@phillip-stephens phillip-stephens requested a review from zakird March 5, 2024 22:01
@phillip-stephens phillip-stephens marked this pull request as ready for review March 5, 2024 22:01
@zakird
Copy link
Member

zakird commented Mar 6, 2024

We need to be careful on the licensing of this code. Can we use strscpy instead on linux?

@droe
Copy link
Contributor

droe commented Mar 6, 2024

Turns out that strlcpy was added to glibc only very recently, in 2.38, exactly the version that Ubuntu 23.10 ships with, which is why it compiled fine for me on Linux. The ifdef __linux__ does not account for strlcpy being available with recent glibc on Linux. My suggestion would be to substitute strlcpy with a strncpy plus termination which is portable.

@phillip-stephens
Copy link
Contributor Author

Great suggestion @droe, I'll do that. I'll also add some automatic compile check tests for different ubuntu release versions (thinking 18.04+) so we can check for this easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants