Skip to content
This repository has been archived by the owner on Sep 20, 2022. It is now read-only.

sniffer: rewrite to be completely byte-based #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Feb 18, 2019

This is intended to speed-up the sniffer (I did not test if it does, just if it works ;-)).

Addresses #56.

@miri64 miri64 requested review from jnohlgard and smlng February 18, 2019 15:35
@miri64 miri64 force-pushed the sniffer/enh/byte-based branch 2 times, most recently from 783918e to 9090744 Compare February 18, 2019 15:38
@miri64
Copy link
Member Author

miri64 commented Mar 19, 2019

Ping @gebart @smlng?

@benpicco benpicco requested a review from chrysn September 16, 2022 11:10
@chrysn
Copy link
Member

chrysn commented Sep 16, 2022

Frankly, when trying out the sniffer application, I didn't even realize that this had a companion application.

As this is now already changing the binary output, is there any particular reason to not already send pcap format, and replace the encoder with cat? 🐱

(There is the time argument, but given that the channel is already set up, absolute time could be set up the same way, and then the relative timing precision should become way better).

Can still review if not (given that it's a python2->python3 update which I have a hard time resisting), just checking beforehand if it's going in the right direction.

@miri64
Copy link
Member Author

miri64 commented Sep 19, 2022

As this is now already changing the binary output, is there any particular reason to not already send pcap format, and replace the encoder with cat? cat

(There is the time argument, but given that the channel is already set up, absolute time could be set up the same way, and then the relative timing precision should become way better).

There is also the issue that for every new sniffer application the data is piped into the device would need to reset to generate a new PCAP file header. I think that the synchronization required for that is more hassle than it is worth.

@chrysn
Copy link
Member

chrysn commented Sep 19, 2022

I don't understand what you mean by "every new sniffer application". Is the expectation that the output of consecutive runs (or parallel runs on different devices) of this application could be concatenated and run through the Python script to produce a single unified PCAP file? If it's just that, the mergecap program shipped with wireshark can resolve that.

@miri64
Copy link
Member Author

miri64 commented Sep 19, 2022

"every new sniffer application".

E.g. piping the output to wireshark or tcpdump.

@miri64
Copy link
Member Author

miri64 commented Sep 19, 2022

If it's just that, the mergecap program shipped with wireshark can resolve that.

Does this also add a PCAP header if the content of the output is (potentially front-truncated) PCAP records?

@chrysn
Copy link
Member

chrysn commented Sep 19, 2022

Sorry, I still don't get it. With the textual format (or the one as implemented in this PR), you spin up a sniffer.py application for each source (because sniffer.py takes a single input), and it produces a pcap file on stdout (which, as I understand, has a header and thus can't be concatenated easily). This is the same situation as if the board produced a PCAP file on its own. Where did I miss a difference?

(looking at the follow-up post)

If a PCAP file is found without its global header, it is not recognized -- and this probably shows the property we haven't talked about but which is relevant: The output format needs to be self-synchronizing because sockets as implemented in typical embedded interfaces typically don't block but just lose initial data.

This is probably the one property that does justify using a custom format here, and which does address my comment.

@chrysn
Copy link
Member

chrysn commented Sep 19, 2022

The README will need an update: "You can check if everything [...] works by connecting to the board via UART and by checking with ifconfig" is not true any more.

@miri64
Copy link
Member Author

miri64 commented Sep 19, 2022

Will retest and change the README, once #78 is merged.

@miri64 miri64 force-pushed the sniffer/enh/byte-based branch from 9090744 to 0783a47 Compare September 19, 2022 14:35
@miri64
Copy link
Member Author

miri64 commented Sep 19, 2022

Rebased to current master and adapted for 2022.07, but it does not work. Looking at the code again, I am also not sure, if this is still a good idea in the current form. We basically restrict the sniffer with this to only be able to handle channels <256 and also only to the default channel page of the device. While the script does not cover pages, the application code does (as it just uses ifconfig). The packet logic might work, but I am unhappy with the configuration logic. So I am inclined to rather not merge this. I will provide a PR for the python 3 fixes though.

@miri64
Copy link
Member Author

miri64 commented Sep 19, 2022

but it does not work.

(the sniffer does not pick up packets was what I meant)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants