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.

Sign up for GitHub

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

Jump to bottom

Allow plugins to use WKS header strings #9643

Merged
merged 2 commits into from
Jun 8, 2023
Merged

Allow plugins to use WKS header strings #9643

merged 2 commits into from
Jun 8, 2023

Conversation

zwoop
Copy link
Contributor

@zwoop zwoop commented Apr 25, 2023

No description provided.

@zwoop zwoop added Plugins TS API header_rewrite header_rewrite plugin labels Apr 25, 2023
@zwoop zwoop added this to the 10.0.0 milestone Apr 25, 2023
@zwoop zwoop self-assigned this Apr 25, 2023
@zwoop zwoop marked this pull request as draft April 25, 2023 19:44
@zwoop zwoop force-pushed the WKS4Plugins branch 4 times, most recently from 3f5babe to 1830f5b Compare April 25, 2023 22:13
bneradt
bneradt requested changes May 1, 2023
View reviewed changes
Copy link
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward.

I think I missed some discussion about this, but just to make sure I understand this PR. Are we adding TSMimeHdrStringToWKS because TSMimeHdrFieldFind is significantly more performant with a wks?

doc/developer-guide/api/functions/TSMimeHdrFieldValueStringGet.en.rst Outdated
@@ -65,6 +65,9 @@ preferred.
value, and populated :arg:`value_len_ptr` with the length of the
value in bytes. The returned header value is not NUL-terminated.

In addition to all the predefined constants for Well-Kown header strings, you can
Copy link
Contributor

Choose a reason for hiding this comment

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

Kown -> Known

@SolidWallOfCode
Copy link
Member

SolidWallOfCode commented May 12, 2023 •
edited
Loading

If we step back a bit, why can't TS_MIME_FIELD_CACHE_CONTROL be the WKS string? Would this then before the WKS that don't have an exported symbol?

What does the lookup return if the string isn't a WKS? nullptr presumably? That should be noted in the documentation.

Otherwise it seems reasonable.

@zwoop
Copy link
Contributor Author

zwoop commented May 14, 2023

Looks pretty straightforward.

I think I missed some discussion about this, but just to make sure I understand this PR. Are we adding TSMimeHdrStringToWKS because TSMimeHdrFieldFind is significantly more performant with a wks?

Yes, significantly faster (no strncmp's).

@zwoop
Copy link
Contributor Author

zwoop commented May 14, 2023 •
edited
Loading

@SolidWallOfCode Not sure I understand. TS_MIME_FIELD_CACHE_CONTROL is the WKS for "Cache-Control", the internal MIME_FIELD_CACHE_CONTROL is set using the exact API that I'm exposing here to plugins.The problem is that there's no way to go from "Cache-Control" to TS_MIME_FIELD_CACHE_CONTROL, and that's what I'm exposing (making public).

Yes, will update the docs with the nullptr return value.

@zwoop
Copy link
Contributor Author

zwoop commented May 14, 2023

Updated the docs, will email the mailing list now with the API proposal.

@ezelkow1
Copy link
Member

[approve ci centos]

@zwoop zwoop marked this pull request as ready for review May 30, 2023 20:00
@zwoop
Copy link
Contributor Author

zwoop commented Jun 2, 2023

@bneradt I fixed the typo, can you review again please?

@zwoop zwoop merged commit 37994a2 into apache:master Jun 8, 2023
@zwoop zwoop deleted the WKS4Plugins branch June 8, 2023 18:56
zwoop added a commit to zwoop/trafficserver that referenced this pull request Jun 15, 2023
@zwoop
Fixes Coverity 1513058, introduced with apache#9643
4ef15c7
zwoop added a commit that referenced this pull request Jun 16, 2023
@zwoop
Fixes Coverity 1513058, introduced with #9643 (#9860)
8595cac
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
@zwoop
Merge remote-tracking branch 'asf/master'
301419f 
* asf/master: (90 commits)
  doc: fix the internal libraries section formatting (apache#9879)
  Add max thread count options to CMake build (apache#9883)
  Add yaml libs reference to HTTP proxy test suite. Closes apache#9882 (apache#9885)
  Add transparent proxy support to CMake build (apache#9884)
  Check for symbol IP_TOS in CMake build (apache#9870)
  RAT license fix: renamed_records.out -> .gold (apache#9876)
  Add traffic_wccp to CMake build (apache#9867)
  cleanup cast warning with reinterpret_cast (apache#9866)
  Fixes Coverity 1513058, introduced with apache#9643 (apache#9860)
  add some missing libs for clang (apache#9865)
  Add support for libunwind in CMake build (apache#9862)
  Add option to build regression tests (apache#9863)
  Fix crash on config reload with BoringSSL (apache#9840)
  Check for SO_PEERCRED in CMake build (apache#9855)
  Check for SO_MARK in CMake build (apache#9854)
  Clean up UnixNetProcessor entanglements. (apache#9825)
  Remove unneeded DEBUG conditionals. (apache#9849)
  Add option to enable fast SDK in CMake build (apache#9853)
  Add support for POSIX Cap in CMake build (apache#9852)
  WCCP: remove ts::Buffer (apache#9824)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bneradt bneradt bneradt approved these changes

Assignees

@zwoop zwoop

Labels
header_rewrite header_rewrite plugin Plugins TS API
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zwoop @SolidWallOfCode @ezelkow1 @bneradt