Skip to content

Conversation

Dimi1010
Copy link
Collaborator

This PR adds discrete FTPControl and FTPData protocol types, while changing FTP to protocol family.

This is done to be able to better distinguish between FTP data channel and FTP control channel as they are normally on different ports. (relevant to #1881)

Potential breaking change:

  • the protocol type value 42 now refers to FTPControl instead of FTP and will only match control payloads.

@Dimi1010 Dimi1010 added refactoring API breaking change Pull request contains a breaking change to the public interface. labels Aug 10, 2025
Copy link

codecov bot commented Aug 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.47%. Comparing base (31f6a05) to head (1770bc1).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1916      +/-   ##
==========================================
- Coverage   83.47%   83.47%   -0.01%     
==========================================
  Files         298      298              
  Lines       53949    53950       +1     
  Branches    11982    11819     -163     
==========================================
  Hits        45033    45033              
+ Misses       7703     7682      -21     
- Partials     1213     1235      +22     
Flag Coverage Δ
alpine320 75.44% <100.00%> (ø)
fedora42 75.56% <100.00%> (ø)
macos-13 81.66% <100.00%> (ø)
macos-14 81.66% <100.00%> (ø)
macos-15 81.66% <100.00%> (ø)
mingw32 70.30% <100.00%> (+0.01%) ⬆️
mingw64 70.29% <100.00%> (ø)
rhel94 75.27% <100.00%> (ø)
ubuntu2004 59.21% <50.00%> (+<0.01%) ⬆️
ubuntu2004-zstd 59.31% <50.00%> (+<0.01%) ⬆️
ubuntu2204 75.23% <100.00%> (+0.02%) ⬆️
ubuntu2204-icpx 60.90% <100.00%> (ø)
ubuntu2404 75.47% <100.00%> (+0.02%) ⬆️
ubuntu2404-arm64 75.45% <100.00%> (ø)
unittest 83.47% <100.00%> (-0.01%) ⬇️
windows-2022 85.48% <100.00%> (ø)
windows-2025 85.49% <100.00%> (ø)
winpcap 85.49% <100.00%> (ø)
xdp 52.97% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Dimi1010 Dimi1010 marked this pull request as ready for review August 10, 2025 09:44
@Dimi1010 Dimi1010 requested a review from seladb as a code owner August 10, 2025 09:44
const ProtocolType FTPData = 60;

/// FTP protocol family (FTPControl and FtpData protocols)
const ProtocolTypeFamily FTP = 0x3c29;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why FTP value is 0x3c29?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It follows the convention for other protocol family packs.

Hex 3c = Dec 60 = FTPData
Hex 29 = Dec 41 = FTPControl

A single protocol value is 8 bits, while a family is 32 bits packed 4x 8 bit protocol type values. Essentially being (protocol1, protocol2, protocol3, protocol4).

The public API takes 32-bit family packs, so if a single protocol is passed, the value gets zero extended to (unknown protocol, unknown protocol, unknown protocol, protocol value).

Ideally, I would have this done with structs and constepxr construction, but we need cpp17 for that to work alongside the current usages inside switch cases. Until then, hard coded magic values it is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow... TBH it's a little hard to get the idea just from the source code. Could you add the comment in the code, thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Sure I can try to add documentation on how it works. Will be in a different PR tho.

@Dimi1010 Dimi1010 merged commit e2d10e9 into seladb:dev Aug 14, 2025
42 checks passed
@Dimi1010 Dimi1010 deleted the refactor/ftp-data-protocol-type branch August 14, 2025 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API breaking change Pull request contains a breaking change to the public interface. refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants