Take string view in mime methods

[hnakamur]

No description provided.

[hnakamur]

Here's the clang-analyzer report concerning a nullptr dereference, in case it's helpful:

https://ci.trafficserver.apache.org/job/Github_Builds/job/clang-analyzer/5704/artifact/output/12167/scan-build-2025-04-07-13-36-30-195012-dqlezm4f/index.html

I suppose this was a false positive from clang-analyzer.

I did 'git bisect' with a script whose content is https://github.com/apache/trafficserver-ci/blob/9a6bb8b40f2a6c830d52f619fa06cda22c60abf4/jenkins/github/clang-analyzer.pipeline#L60-L91
and found the commit 99a15bf was the trigger.

However I did not think the commit was actual cause after reviewing the diff.
I think clang-analyzer was confused that string_get may return nullptr because of if (new_url == nullptr) below it.

new_url = to_free = u->string_get(&s->arena, &new_url_len);
if (new_url == nullptr) {
  new_url = "";
}

I suppose that string_get always returns non nullptr.
So I added assert(to_free != nullptr); at 038258a and clang-analyzer does not report nullptr dereference anymore.

If this assumption is true, the following three lines are not needed.

if (new_url == nullptr) {
  new_url = "";
}

If this assumption is false, we must guard the call to str_free like below:

if (to_free) {
  s->arena.str_free(to_free);
}

[cmcfarlen]

I like the idea of this cleanup, but I think if we kept the old functions and just added overrides then there would be fewer incidental conversions to string_view.

[cmcfarlen]

I think for this one and a few others we could provide an override that takes string_view rather than replace the existing one. That will avoid some of the conversions to string_view that just get converted back immediately.

[hnakamur]

I thought we could change lower-level functions such as mime_hdr_prepare_for_value_set to take string_view once this pull request is merged.

[hnakamur]

I wonder should I change lower-level functions to take string_view before this pull request?
Or should I revert removing the version taking const char * and int as you suggested?

[hnakamur]

I changed lower level functions to take string_view up to the commit 673b9e5.

[cmcfarlen]

This is another example where also having the other override is cleaner.

[hnakamur]

I thought we could add string_view version of mime field constants once this pull request is merged.
After that, we could deprecate const char * MIME_FIELD_XXX and int MIME_LEN_XXX.

[hnakamur]

I've made the changes I said above at 038258a...hnakamur:trafficserver:take_string_view_in_mime_methods2

[hnakamur]

I added c_str_view which is similar to std::string_view but asserts the string is terminate with \0.
It is needed in src/api/InkAPIInternal.cc like:

TS_MIME_FIELD_ACCEPT                    = MIME_FIELD_ACCEPT.c_str();

The type of TS_MIME_FIELD_ACCEPT is const char * and it expected the string is terminated with \0.
The type of MIME_FIELD_ACCEPT was changed to c_str_view which expresses so.

[hnakamur]

I merged master to resolve conflicts caused by #12091 being merged.

[cmcfarlen]

One more change request just for commentary. Thanks for doing this!

[cmcfarlen]

I think this is ok, BUT, please add a comment here that the purpose of this class is to transition between older code that uses char * and cleaned up code that uses std::string_view and it will be removed when it is not needed.

We should convert the wks code to be more C++ and then remove this.

[hnakamur]

Thank you for your comment. I added a comment at 629bdf3.

[cmcfarlen]

sv_.data()[sv_.length()] == '\0' could be considered UB if the string_view is not null terminated as this might be reading past a buffer, but since this is an assert anyway its fine if it crashes when called with a non-null terminated string_view.