Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LLDP add PoE TLV #4346

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

LLDP add PoE TLV #4346

wants to merge 1 commit into from

Conversation

matsievskiysv
Copy link
Contributor

@matsievskiysv matsievskiysv commented Apr 9, 2024

Checklist:

  • If you are new to Scapy: I have checked CONTRIBUTING.md (esp. section submitting-pull-requests)
  • I squashed commits belonging together
  • I added unit tests or explained why they are not relevant
  • I executed the regression tests (using cd test && ./run_tests or tox)
  • If the PR is still not finished, please create a Draft Pull Request

This PR adds PoE (IEEE802.3bt) organization specific TLVs for LLDP layer

Different versions of PoE TLVs were added as separate classes instead of using ConditionalField to allow the user explicitly instantiate the desired TLV version.

@matsievskiysv
Copy link
Contributor Author

Unit test failed because tshark is not installed on testing machine

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Merging #4346 (b736503) into master (0a2b2bc) will decrease coverage by 0.53%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4346      +/-   ##
==========================================
- Coverage   82.03%   81.50%   -0.53%     
==========================================
  Files         350      350              
  Lines       82888    83230     +342     
==========================================
- Hits        67994    67836     -158     
- Misses      14894    15394     +500     
Files Coverage Δ
scapy/contrib/lldp.py 94.62% <100.00%> (+2.09%) ⬆️

... and 38 files with indirect coverage changes

@matsievskiysv matsievskiysv marked this pull request as draft April 10, 2024 12:17
@matsievskiysv matsievskiysv marked this pull request as ready for review April 11, 2024 05:38
@matsievskiysv
Copy link
Contributor Author

Added PoE measure TLV. Again, macos tests are failing because tshark is not installed.

Copy link
Contributor

@polybassa polybassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR. Please make use of ScalingFields for physical units

scapy/contrib/lldp.py Outdated Show resolved Hide resolved
scapy/contrib/lldp.py Outdated Show resolved Hide resolved
scapy/contrib/lldp.py Outdated Show resolved Hide resolved
scapy/contrib/lldp.py Outdated Show resolved Hide resolved
scapy/contrib/lldp.py Outdated Show resolved Hide resolved
@matsievskiysv
Copy link
Contributor Author

matsievskiysv commented Apr 16, 2024

Changed to use ScalingField everywhere. Yet again, macos check failed because tshark is not installed.

polybassa
polybassa previously approved these changes Apr 17, 2024
@matsievskiysv matsievskiysv marked this pull request as draft April 18, 2024 06:55
@matsievskiysv
Copy link
Contributor Author

Found the mistake in LLDPDUPowerViaMDI length. Fixed it and added tshark tests for this TLV (initially I thought it was unsupported by Wireshark)

@matsievskiysv matsievskiysv marked this pull request as draft April 18, 2024 07:19
@matsievskiysv matsievskiysv marked this pull request as ready for review April 18, 2024 07:29
@matsievskiysv
Copy link
Contributor Author

Double-checked LLDPDUPowerViaMDIMeasure via Wireshark. For some strange reason, it reports the warning Invalid length, greather then expected even though package length, counting from Org code, is 26 octets long.

@gpotter2 gpotter2 self-assigned this May 9, 2024
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.

None yet

4 participants