implement poll() for `startCaptureBlockingMode` #1245

tigercosmos · 2023-11-24T08:30:26Z

please refer to #1220

new in PR:

timeout of startCaptureBlockingMode keeps the unit as second but changes to use double type to allow millisecond precision.
add usePoll option
main behavior difference: originally, if there are no packets coming, then the timeout will never be triggered due to blocking of pcap_dispatch. Use poll() to frequently check if it is already timeout.

tigercosmos · 2023-11-24T08:30:43Z

@seladb please take a look.

Pcap++/src/PcapLiveDevice.cpp

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp

seladb · 2023-11-25T02:01:58Z

@tigercosmos I updated the base branch to dev. We always merge PRs to dev first before merging them to master.

Please also note CI fails on many platforms. Can you please check?

tigercosmos · 2023-11-25T03:03:31Z

@tigercosmos I updated the base branch to dev. We always merge PRs to dev first before merging them to master.

Please also note CI fails on many platforms. Can you please check?

@seladb Sure, I will fix the CI. I would like to ask your opinion about this PR. Does the current implementation make sense to you?

seladb

Please see a few initial comments. I will do a more thorough review once CI passes

Pcap++/src/PcapLiveDevice.cpp

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp

tigercosmos · 2023-11-27T19:08:41Z

@seladb I think it's ready for review

Tests/Pcap++Test/TestDefinition.h

tigercosmos · 2023-12-06T10:58:19Z

it's weird, it seems that iptables does not work by default, but it succeeded before: 1a50a03

tigercosmos · 2023-12-07T05:00:45Z

@seladb I try to use sudo in the std::system and also try to use sudo to start the test. Neither of them work:

iptables v1.6.1: can't initialize iptables table `filter': Permission denied (you must be root)
Perhaps iptables or your kernel needs to be upgraded.
iptables v1.6.1: can't initialize iptables table `filter': Permission denied (you must be root)
Perhaps iptables or your kernel needs to be upgraded.

Moreover, currently CI containers complain there is no iptables, so we also need to install it.
However, before a couple of commits merged into dev, I already used iptables, and CI was totally fine.

I also check the changes on dev, it seems it should not affect this "cannot use iptables".

seladb · 2023-12-07T05:16:48Z

@seladb I try to use sudo in the std::system and also try to use sudo to start the test. Neither of them work:
iptables v1.6.1: can't initialize iptables table `filter': Permission denied (you must be root)
Perhaps iptables or your kernel needs to be upgraded.
iptables v1.6.1: can't initialize iptables table `filter': Permission denied (you must be root)
Perhaps iptables or your kernel needs to be upgraded.
Moreover, currently CI containers complain there is no iptables, so we also need to install it. However, before a couple of commits merged into dev, I already used iptables, and CI was totally fine.

I also check the changes on dev, it seems it should not affect this "cannot use iptables".

Maybe it's not possible to run iptables inside a docker container: https://forums.docker.com/t/iptables-permission-denied/81211/3 😕

tigercosmos · 2023-12-07T07:13:37Z

@seladb I try to use sudo in the std::system and also try to use sudo to start the test. Neither of them work:
iptables v1.6.1: can't initialize iptables table `filter': Permission denied (you must be root)
Perhaps iptables or your kernel needs to be upgraded.
iptables v1.6.1: can't initialize iptables table `filter': Permission denied (you must be root)
Perhaps iptables or your kernel needs to be upgraded.
Moreover, currently CI containers complain there is no iptables, so we also need to install it. However, before a couple of commits merged into dev, I already used iptables, and CI was totally fine.
I also check the changes on dev, it seems it should not affect this "cannot use iptables".
Maybe it's not possible to run iptables inside a docker container: https://forums.docker.com/t/iptables-permission-denied/81211/3 😕

True... after survey, seems that Github action has some limitations.
seems a possible solution is to run a docker inside Github action with --cap-add=NET_ADMIN

Or, probably we should think of some other ways to test the function.

tigercosmos · 2023-12-07T08:25:49Z

@seladb Maybe we should not make it very complicated... as long as the poll version can work the same as the original version, maybe we don't need to prove that poll can timeout with no packets coming (we should believe that poll is always correct)

seladb · 2023-12-08T07:21:58Z

@seladb Maybe we should not make it very complicated... as long as the poll version can work the same as the original version, maybe we don't need to prove that poll can timeout with no packets coming (we should believe that poll is always correct)

@tigercosmos I tend to agree... I can't think of a good way to test the poll timeout so let's just keep the existing tests. It's not ideal but we currently don't have a CI infra that is robust enough to test this scenario.

Can you clean up the PR and I'll review it again?

tigercosmos · 2023-12-08T08:33:18Z

Pcap++/src/PcapLiveDevice.cpp

-				m_StopThread = true;
+#if !defined(_WIN32)
+				// Be careful when you modify the code related to `poll` in this block
+				// Some tests are not supported to run on CIs: `TestPcapLiveDeviceBlockingModePollTimeout`, `TestPcapLiveDeviceBlockingModeNotTimeoutWithoutPoll`


@seladb I added some comments here to mention that manual tests are required here

tigercosmos · 2023-12-08T08:34:17Z

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp

+
+#if !defined(_WIN32)
+	configs.emplace_back(); // the config used poll
+	configs[1].usePoll = true;


this ensures the behaviour is as same as before.

tigercosmos · 2023-12-08T08:35:59Z

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp

+
+PTF_TEST_CASE(TestPcapLiveDeviceBlockingModePollTimeout)
+{
+#if !defined(_WIN32) and defined(MANUAL_TEST)


although this test cannot run on CI, we can manually run it locally.
I think it is worth keeping it, so I added a MANUAL_TEST flag for it.

I found that it's really difficult to use iptables to block all packets. No matter how I set up rules, I still can see packets on Wireshark. Maybe the easiest way is to use a clean interface to test... since we can ensure no packets on it ...

@seladb please let me know your opinion

Yes, I also encountered problems with iptables. I think we should just remove these tests. There is no point in MANUAL_TEST which we don't run anywhere...

tigercosmos · 2023-12-08T08:36:15Z

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp

-	PTF_ASSERT_TRUE(liveDev->startCapture(packetArrives, &packetCount));
+PTF_TEST_CASE(TestPcapLiveDeviceBlockingModeNotTimeoutWithoutPoll)
+{
+#if !defined(_WIN32) and defined(MANUAL_TEST)


same for this test. use MANUAL_TEST here.

I think we can remove this test also...

seladb · 2023-12-12T06:52:14Z

Pcap++/src/PcapLiveDevice.cpp

+				else if(ready < 0)
+				{
+					PCPP_LOG_ERROR("poll() got error '" <<  strerror(errno) << "'");
+					return -1;


I don't think we can return 0 directly because in lines 597-600 we reset a few private members.
Instead we should set m_StopThread = true and maybe add a new local variable pollError and I think we can return 0 in this case (poll error)

seladb · 2023-12-12T06:52:53Z

Pcap++/src/PcapLiveDevice.cpp

+				}
+#else
+				PCPP_LOG_ERROR("Windows doesn't support poll()");
+				return 0;


ditto: we cannot return 0 here. We should probably add a new local variable pollError and set it to true

seladb · 2023-12-12T06:55:09Z

Pcap++/src/PcapLiveDevice.cpp

 	{
 		while (!m_StopThread)
 		{
-			if (pcap_dispatch(m_PcapDescriptor, -1, onPacketArrivesBlockingMode, reinterpret_cast<uint8_t*>(this)) == -1)
+			if(pcap_dispatch(m_PcapDescriptor, -1, onPacketArrivesBlockingMode, (uint8_t*)this) < 0)


An error in pcap_dispatch is -1 and not other values:

https://linux.die.net/man/3/pcap_dispatch

It returns -1 if an error occurs or -2 if the loop terminated due to a call to pcap_breakloop() before any packets were processed. If your application uses pcap_breakloop(), make sure that you explicitly check for -1 and -2, rather than just checking for a return value < 0.

sorry, fixed.

seladb · 2023-12-12T07:00:09Z

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp

+
+PTF_TEST_CASE(TestPcapLiveDeviceBlockingModePollTimeout)
+{
+#if !defined(_WIN32) and defined(MANUAL_TEST)


Yes, I also encountered problems with iptables. I think we should just remove these tests. There is no point in MANUAL_TEST which we don't run anywhere...

seladb · 2023-12-12T07:00:23Z

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp

-	PTF_ASSERT_TRUE(liveDev->startCapture(packetArrives, &packetCount));
+PTF_TEST_CASE(TestPcapLiveDeviceBlockingModeNotTimeoutWithoutPoll)
+{
+#if !defined(_WIN32) and defined(MANUAL_TEST)


I think we can remove this test also...

tigercosmos · 2023-12-13T02:01:33Z

@seladb all addressed

tigercosmos · 2023-12-13T07:22:17Z

finally 🎉
thanks @seladb

seladb · 2023-12-13T08:48:54Z

finally 🎉 thanks @seladb

Thank you @tigercosmos for your hard work and patience in solving all the issues, very much appreciated! 🙏

tigercosmos and others added 13 commits November 21, 2023 04:56

wip

e383a0d

fix tidy

2d51095

CI

bf88d15

fix build

207f362

doc

1ab0abd

improve poll timeout

1568c86

test timeout

b5e5f60

try to fix test

094479d

fix the behavior

06136dc

save some lines

d7e4630

fix tidy

1c98e4d

should fix the behavior

358b568

pcap with poll

6480fac

tigercosmos requested a review from seladb as a code owner November 24, 2023 08:30

tigercosmos commented Nov 24, 2023

View reviewed changes

Pcap++/src/PcapLiveDevice.cpp Outdated Show resolved Hide resolved

tigercosmos commented Nov 24, 2023

View reviewed changes

Pcap++/src/PcapLiveDevice.cpp Outdated Show resolved Hide resolved

tigercosmos commented Nov 24, 2023

View reviewed changes

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp Outdated Show resolved Hide resolved

seladb changed the base branch from master to dev November 25, 2023 01:55

seladb and others added 2 commits November 24, 2023 17:56

Merge branch 'dev' into blockingCall

7c08824

fix cross platform build

1d6f804

seladb reviewed Nov 25, 2023

View reviewed changes

Pcap++/src/PcapLiveDevice.cpp Show resolved Hide resolved

Pcap++/src/PcapLiveDevice.cpp Outdated Show resolved Hide resolved

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp Outdated Show resolved Hide resolved

tigercosmos commented Nov 26, 2023

View reviewed changes

Tests/Pcap++Test/Tests/LiveDeviceTests.cpp Show resolved Hide resolved

tigercosmos and others added 4 commits November 27, 2023 09:31

Merge branch 'dev' into blockingCall

f76d432

try to fix error

367ba95

fix

6feecda

fix ci

c219834

seladb reviewed Dec 6, 2023

View reviewed changes

Tests/Pcap++Test/TestDefinition.h Outdated Show resolved Hide resolved

tigercosmos and others added 2 commits December 6, 2023 17:31

add missing PTF_RUN_TEST

7120404

no need sudo

f80953a

Merge branch 'dev' into blockingCall

a280f25

tigercosmos added 3 commits December 8, 2023 17:18

revert

430cd6f

add MANUAL_TEST

7671f77

revert

856f784

tigercosmos commented Dec 8, 2023

View reviewed changes

tigercosmos added 3 commits December 8, 2023 18:02

fix

5f4b9dc

fix

cd4f6e6

fix

5938c68

seladb reviewed Dec 12, 2023

View reviewed changes

tigercosmos added 3 commits December 13, 2023 09:20

review.

620d015

remove unused include

0a074d1

update

497888a

seladb approved these changes Dec 13, 2023

View reviewed changes

seladb merged commit 660843e into seladb:dev Dec 13, 2023
39 checks passed

tigercosmos deleted the blockingCall branch December 13, 2023 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement poll() for `startCaptureBlockingMode` #1245

implement poll() for `startCaptureBlockingMode` #1245

tigercosmos commented Nov 24, 2023 •

edited

Loading

tigercosmos commented Nov 24, 2023

seladb commented Nov 25, 2023

tigercosmos commented Nov 25, 2023 •

edited

Loading

seladb left a comment

tigercosmos commented Nov 27, 2023

tigercosmos commented Dec 6, 2023

tigercosmos commented Dec 7, 2023

seladb commented Dec 7, 2023

tigercosmos commented Dec 7, 2023

tigercosmos commented Dec 7, 2023

seladb commented Dec 8, 2023

tigercosmos Dec 8, 2023

tigercosmos Dec 8, 2023

tigercosmos Dec 8, 2023

tigercosmos Dec 8, 2023

tigercosmos Dec 11, 2023

seladb Dec 12, 2023

tigercosmos Dec 8, 2023

seladb Dec 12, 2023

seladb Dec 12, 2023

tigercosmos Dec 13, 2023

seladb Dec 12, 2023

tigercosmos Dec 13, 2023

seladb Dec 12, 2023

tigercosmos Dec 13, 2023

seladb Dec 12, 2023

seladb Dec 12, 2023

tigercosmos commented Dec 13, 2023

tigercosmos commented Dec 13, 2023

seladb commented Dec 13, 2023

implement poll() for startCaptureBlockingMode #1245

implement poll() for startCaptureBlockingMode #1245

Conversation

tigercosmos commented Nov 24, 2023 • edited Loading

tigercosmos commented Nov 24, 2023

seladb commented Nov 25, 2023

tigercosmos commented Nov 25, 2023 • edited Loading

seladb left a comment

Choose a reason for hiding this comment

tigercosmos commented Nov 27, 2023

tigercosmos commented Dec 6, 2023

tigercosmos commented Dec 7, 2023

seladb commented Dec 7, 2023

tigercosmos commented Dec 7, 2023

tigercosmos commented Dec 7, 2023

seladb commented Dec 8, 2023

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

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

tigercosmos commented Dec 13, 2023

tigercosmos commented Dec 13, 2023

seladb commented Dec 13, 2023

implement poll() for `startCaptureBlockingMode` #1245

implement poll() for `startCaptureBlockingMode` #1245

tigercosmos commented Nov 24, 2023 •

edited

Loading

tigercosmos commented Nov 25, 2023 •

edited

Loading