Netdump library #6518

hreintke · 2019-09-14T14:08:53Z

Netdump library, initial commit

dok-net · 2019-09-16T17:37:18Z

@hreintke There's not a single patch to the ESP8266 Arduino core in the PR, it's a plain old library from what I see, why don't you just make it an Arduino library, available via the library manager? If there's interest in the core team, they can still pick it up as a git submodule and establish fixed version relationships in this way, I would not encourage that, though, speaking from experience with EspSoftwareSerial.
The added benefit is that you won't have to bother writing PRs for every change and release you make to/for NetdumpLibrary.

hreintke · 2019-09-17T07:35:45Z

@dok-net At the request of core developers
cc @d-a-v

dok-net · 2019-09-17T10:14:23Z

@hreintke I'm just commenting out of concern over ever-longer build times.
That left aside, a review remark, if that's appropriate, other internal libs use, in their library.properties:

dot_a_linkage=true

hreintke · 2019-09-17T10:56:28Z

@dok-net No problem, your comments. And review remarks are welcome.
On longer build time : Aren't libraries only build when included/used in the sketch ?
@d-a-v : Should I include library.properties: dot_a_linkage=true

d-a-v · 2019-09-17T13:08:58Z

Unused libraries are not compiled.
dot_a_linkage=true is harmless and useful if this library stays here.
This library depends on phy_capture only existing in this core, in fact in another submodule of this core.
Indeed I suggested @hreintke to propose his great improvements of my simple netdump directly here as a PR. However I personally have absolutely nothing against this library from being a submodule or a separate library (it can be useful with Ethernet, but a patch on this one is needed to propose a phy-capture-like call). It's @hreintke's call.

earlephilhower · 2019-09-17T17:49:13Z

Re: the longer build times, there is a bug in the arduino-builder that was recently patched that made it scan the entire .git tree (takes 2+ mins and 4GB on my server!) at the start of every build. arduino/arduino-cli#366 . The nightly arduino tarball should have this fixed, or you can build your own arduino-builder executable and install over the existing one from a stable release. This only applies, of course, when you're building from a git clone'd copy and not a board manager install.

earlephilhower · 2019-09-17T18:01:01Z

@hreintke trivial failure on the example. Can you run tests/restyle.sh to clean up the formatting to match Arduino standard?

hreintke · 2019-09-17T22:28:26Z

@earlephilhower
This is the initial commit. There is work to do for improving efficiency (using cost type& parameters).
I am working on that at the moment.
In the next update I will also include the style changes.

dok-net · 2019-09-18T09:27:02Z

About build times for libraries, what I mostly had in mind is CI, where the examples are all built. Is the NetdumpLibrary really this essential?
"Pollution" of the ESP8266 core git history would be another issue - a submodule over standalone library still has some of the disadvantage as you can tell from the frequent EspSoftwareSerial PRs ;-)
And of course, there is a tendency to just use whatever is there, sometimes resulting in dependency loops or core depending on library.

dok-net · 2019-09-18T09:55:01Z

@hreintke IIRC, examples should be in <lib>/examples/<exampleproj>/<exampleproj>.ino
Plus, from the back of my mind, <exampleproj> != <lib>

devyte · 2019-11-08T17:53:52Z

@hreintke could you please fix this PR?

hreintke · 2019-11-09T15:50:01Z

@devyte OK, will fix the issues on the PR.
Do you still want it included in this repo or do you want it as an external library.

d-a-v · 2019-11-09T15:58:02Z

Do you still want it included in this repo or do you want it as an external library.

It's up to you.

If it is an external library you will be the maintainer of it, others will PR on it, we'll import it as a submodule, and it will be usable with other network library if they implement/have a phy_capture() like API.

If it is internal, you'll be able to fix/improve it with PRs.

Anyway, as a submodule or plainly included, it will be useful for learning and debugging.

devyte · 2019-11-09T16:24:58Z

@hreintke there are ongoing networking changes and development, and I expect users will be instructed to make use of it for dumps, so I prefer it in the repo close at hand, at least for now.

…into NetdumpLibrary

hreintke · 2019-11-12T11:22:59Z

@d-a-v @devyte
Had some trouble/unexpected behavior on my github Netdump branch.
Think it is now up to date with my development.

I updated with the changes for const etc. Can you check those and comment ?
Can you give a astyle conf, then I will apply that
Working now on further test and completion of example

d-a-v · 2019-11-12T12:36:32Z

Can you give a astyle conf, then I will apply that

Add libraries/Netdump into this line and run the script.

libraries/Netdump/library.properties

libraries/Netdump/astyle_goodies.conf

hreintke · 2019-11-26T14:58:58Z

@d-a-v @devyte
Can I use experimental features in Netdump ?
Used CallbackLIst for implementing callbacks to multiple instances of Netdump.
If OK, I will do further testing and cleanup of that.

devyte · 2019-11-26T15:13:48Z

@hreintke experimental means beware it might abruptly change and is not fully tested. In other words, it's ok to use internally for our core features, but not ok for exposing to users as supported functionality/api.
The purpose is in part your use case, so go ahead.

hreintke · 2019-12-05T12:00:42Z

@devyte @d-a-v
Can you do another review ?

libraries/Netdump/examples/Netdump/Netdump.ino

libraries/Netdump/src/NetdumpPacket.cpp

Improve mDNS printf

SPIFFS is deprecated

d-a-v

Thanks !

devyte

Of my comments/questions in my previous review, there are 6 that I think are still valid. All others are resolved.
This review adds 6 additional comments on top of the previous 6.

devyte · 2020-08-12T22:36:35Z

libraries/Netdump/examples/Netdump/Netdump.ino

+  HTTPChar
+};
+
+void startSerial(int option) {


The switch was changed, but the arg is still received as an int.

devyte · 2020-08-12T22:39:31Z

libraries/Netdump/examples/Netdump/Netdump.ino

+  WiFi.begin(ssid, password);
+
+  if (WiFi.waitForConnectResult() != WL_CONNECTED) {
+    Serial.println("WiFi Failed");


Given that what comes next is an infinite loop, I suggest: "WiFiFailed, stopping sketch". I've been confused by these things myself.

devyte · 2020-08-12T22:48:16Z

libraries/Netdump/src/Netdump.h

+    void tcpDumpProcess(const NetdumpPacket& np);
+    void tcpDumpLoop(WiFiServer &tcpDumpServer, NetdumpFilter nf);
+
+    WiFiClient tcpDumpClient;


Does it also not apply to WiFiClientSecure?

devyte · 2020-08-12T22:48:56Z

libraries/Netdump/src/Netdump.h

+    char* packetBuffer = nullptr;
+    size_t bufferIndex = 0;
+
+    static constexpr int tcpBuffersize = 2048;


tcpBufferSize* (uppercase S)

devyte · 2020-08-12T22:51:55Z

libraries/Netdump/src/NetdumpIP.cpp

+NetdumpIP::NetdumpIP()
+{
+}
+
+NetdumpIP::~NetdumpIP()
+{
+}


What I mean is that these two implement the default behavior, i.e.: they are empty, so you don't need to do it yourself, because the compiler provides them for you. Just remove them (cleaner)

devyte · 2020-08-12T22:53:22Z

libraries/Netdump/src/NetdumpIP.cpp

+
+NetdumpIP::NetdumpIP(uint8_t first_octet, uint8_t second_octet, uint8_t third_octet, uint8_t fourth_octet)
+{
+    setV4();


What I mean is that there doesn't seem to be an equivalent for IPv6, and also: is this constructor valid for the case of a sketch with only IPv6? If the answer to that is "no", then should the constructor have different args? have an overload with different args? something else?

devyte · 2020-08-12T23:57:55Z

libraries/Netdump/src/NetdumpPacket.h

+    {
+    	return len;
+    }
+    uint16_t ntoh16(uint16_t idx) const


I meant: rather than reimplement here, why not call the lwip functions directly? or wrap the lwip calls in these methods.

devyte · 2020-08-13T00:20:28Z

libraries/Netdump/src/PacketType.cpp

+PacketType::PacketType()
+{
+}
+
+PacketType::~PacketType()
+{
+}


No need to implement default constructor/destructor, the compiler provides them.

devyte · 2020-08-13T00:21:10Z

libraries/Netdump/src/PacketType.h

+        UKNW,
+    };
+
+    PacketType();


No need for this, if the constructor is the default (empty body in the cpp)

devyte · 2020-08-13T00:21:31Z

libraries/Netdump/src/PacketType.h

+    PacketType();
+    PacketType(PType pt) : ptype(pt) {};
+
+    ~PacketType();


No need for this, if the destructor is the default (empty body in the cpp)

…into NetdumpLibrary

hreintke · 2020-08-14T09:58:36Z

Something did not go the way I wanted.

My goal was to sync my branch with the littleFS/unused parameter patches from @d-a-v.
What happened that the PR is updated with my NetdumpV2 (wip), which restructures the way info is taken from the packets and includes a wifi promiscuous mode.

Not sure now how to continue.
It there an (hopefully easy) to revert the PR to the previous commit ?

Netdump library, initial commit

694732f

d-a-v mentioned this pull request Sep 17, 2019

Remove SoftwareSerial as submodule #6530

Closed

Merge branch 'master' into NetdumpLibrary

c9ce6e8

hreintke added 2 commits November 12, 2019 11:43

Const update

d17f83d

Merge branch 'NetdumpLibrary' of https://github.com/hreintke/Arduino …

b1912a8

…into NetdumpLibrary

hreintke closed this Nov 12, 2019

hreintke deleted the NetdumpLibrary branch November 12, 2019 10:54

hreintke reopened this Nov 12, 2019

d-a-v reviewed Nov 12, 2019

View reviewed changes

libraries/Netdump/library.properties Outdated Show resolved Hide resolved

hreintke added 2 commits November 12, 2019 16:11

Cleanup, added example

da5307f

Example should start Serial by default.

9545dd3

d-a-v reviewed Nov 12, 2019

View reviewed changes

libraries/Netdump/astyle_goodies.conf Outdated Show resolved Hide resolved

hreintke added 2 commits November 13, 2019 16:13

Further cleanup

11426ee

Add PacketType & restructure printing using that

9915230

hreintke added 5 commits November 21, 2019 16:16

Merge branch 'master' into NetdumpLibrary

e7886ce

Restructure PacketType

f05a6f3

Restructure printing, add PacketDetail::RAW

328f240

Fix printing

a5b3ad7

Use CallbackList to facilitate multiple Netdump instances

6f4fb0c

hreintke added 2 commits December 5, 2019 12:59

Separate commit to show update on experimental CallbackList

112c039

Further cleanup

4f099cb

d-a-v reviewed Feb 29, 2020

View reviewed changes

libraries/Netdump/examples/Netdump/Netdump.ino Outdated Show resolved Hide resolved

d-a-v mentioned this pull request Mar 5, 2020

Netdump: tcpdump-like internal library #4678

Closed

Style update and small changes

3d41d25

d-a-v reviewed May 3, 2020

View reviewed changes

libraries/Netdump/src/NetdumpPacket.cpp Show resolved Hide resolved

hreintke and others added 8 commits May 24, 2020 12:21

wip

1b77c97

Merge branch 'master' into NetdumpLibrary

4398807

Use std::unique_ptr to prevent memory leak

d1a142b

Add LLMR

5803720

Improve mDNS printf

Merge branch 'master' into NetdumpLibrary

89485e8

use LittleFS

a0f1e5a

SPIFFS is deprecated

Merge branch 'master' into NetdumpLibrary

8e72ecb

fixed "unused parameter"

9be3d13

d-a-v self-requested a review August 12, 2020 19:42

d-a-v approved these changes Aug 12, 2020

View reviewed changes

devyte requested changes Aug 13, 2020

View reviewed changes

Merge branch 'NetdumpLibrary' of https://github.com/hreintke/Arduino …

5cdee28

…into NetdumpLibrary

hreintke closed this Aug 14, 2020

This was referenced Aug 15, 2020

Netdump library #7526

Closed

Netump Initial commit #7527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Netdump library #6518

Netdump library #6518

hreintke commented Sep 14, 2019

dok-net commented Sep 16, 2019

hreintke commented Sep 17, 2019

dok-net commented Sep 17, 2019

hreintke commented Sep 17, 2019

d-a-v commented Sep 17, 2019 •

edited

earlephilhower commented Sep 17, 2019

earlephilhower commented Sep 17, 2019

hreintke commented Sep 17, 2019

dok-net commented Sep 18, 2019

dok-net commented Sep 18, 2019 •

edited

devyte commented Nov 8, 2019

hreintke commented Nov 9, 2019

d-a-v commented Nov 9, 2019 •

edited

devyte commented Nov 9, 2019

hreintke commented Nov 12, 2019 •

edited

d-a-v commented Nov 12, 2019

hreintke commented Nov 26, 2019

devyte commented Nov 26, 2019

hreintke commented Dec 5, 2019

d-a-v left a comment

devyte left a comment

devyte Aug 12, 2020

devyte Aug 12, 2020

devyte Aug 12, 2020

devyte Aug 12, 2020

devyte Aug 12, 2020

devyte Aug 12, 2020

devyte Aug 12, 2020

devyte Aug 13, 2020

devyte Aug 13, 2020

devyte Aug 13, 2020

hreintke commented Aug 14, 2020

Netdump library #6518

Netdump library #6518

Conversation

hreintke commented Sep 14, 2019

dok-net commented Sep 16, 2019

hreintke commented Sep 17, 2019

dok-net commented Sep 17, 2019

hreintke commented Sep 17, 2019

d-a-v commented Sep 17, 2019 • edited

earlephilhower commented Sep 17, 2019

earlephilhower commented Sep 17, 2019

hreintke commented Sep 17, 2019

dok-net commented Sep 18, 2019

dok-net commented Sep 18, 2019 • edited

devyte commented Nov 8, 2019

hreintke commented Nov 9, 2019

d-a-v commented Nov 9, 2019 • edited

devyte commented Nov 9, 2019

hreintke commented Nov 12, 2019 • edited

d-a-v commented Nov 12, 2019

hreintke commented Nov 26, 2019

devyte commented Nov 26, 2019

hreintke commented Dec 5, 2019

d-a-v left a comment

Choose a reason for hiding this comment

devyte left a 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

hreintke commented Aug 14, 2020

d-a-v commented Sep 17, 2019 •

edited

dok-net commented Sep 18, 2019 •

edited

d-a-v commented Nov 9, 2019 •

edited

hreintke commented Nov 12, 2019 •

edited