Skip to content
New issue

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

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

Already on GitHub? # to your account

Eliminate deprecated lib records apis #12060

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

hnakamur
Copy link
Contributor

@hnakamur hnakamur force-pushed the eliminate_deprecated_lib_records_apis_generic branch 2 times, most recently from e6e7852 to 9c4a6e8 Compare March 1, 2025 14:15
@hnakamur hnakamur force-pushed the eliminate_deprecated_lib_records_apis_generic branch from 9c4a6e8 to 5b728d3 Compare March 1, 2025 14:55
@hnakamur hnakamur force-pushed the eliminate_deprecated_lib_records_apis_generic branch from 5b728d3 to 09744e6 Compare March 1, 2025 15:19
@hnakamur hnakamur force-pushed the eliminate_deprecated_lib_records_apis_generic branch from 09744e6 to 4d8bd36 Compare March 3, 2025 02:19
@cmcfarlen cmcfarlen added this to the 10.1.0 milestone Mar 3, 2025
@bryancall
Copy link
Contributor

[approve ci autest 2]

@bryancall bryancall requested a review from zwoop March 3, 2025 23:17
@bryancall
Copy link
Contributor

Do we want to return value instead of passing in a pointer and return a std::string_view instead of a char*?

@hnakamur hnakamur changed the title Eliminate deprecated lib records apis generic Eliminate deprecated lib records apis Mar 4, 2025
@hnakamur
Copy link
Contributor Author

hnakamur commented Mar 4, 2025

RecEstablishStaticConfig* family needs a pointer to the config value

RecEstablishStaticConfig* family have to take a pointer since RecLinkConfig* family take a pointer.

RecErrT RecLinkConfigInt(const char *name, RecInt *rec_int);
RecErrT RecLinkConfigInt32(const char *name, int32_t *p_int32);
RecErrT RecLinkConfigUInt32(const char *name, uint32_t *p_uint32);
RecErrT RecLinkConfigFloat(const char *name, RecFloat *rec_float);
RecErrT RecLinkConfigCounter(const char *name, RecCounter *rec_counter);
RecErrT RecLinkConfigString(const char *name, RecString *rec_string);
RecErrT RecLinkConfigByte(const char *name, RecByte *rec_byte);

For example, RecEstablishStaticConfigStringAlloc calls RecLinkConfigString.
https://github.com/hnakamur/trafficserver/blob/4d8bd3638cc0683b12773bedf0a6a27df6e109af/src/records/RecCore.cc#L915-L922

RecErrT
RecEstablishStaticConfigStringAlloc(const char *name, RecString *rec_string, bool lock)
{
  if (RecLinkConfigString(name, rec_string) == REC_ERR_OKAY) {
    ats_free(*rec_string);
  }
  return RecGetRecordStringOrNullptr_Xmalloc(name, rec_string, lock);
}

other functions can be changed to return the config value and take a pointer to RecErrT err or bool found

As for other functions, there are few use cases which needs to know wether the config is found or not, for example:
https://github.com/hnakamur/trafficserver/blob/4d8bd3638cc0683b12773bedf0a6a27df6e109af/src/mgmt/config/FileManager.cc#L280-L281

int  enabled;
bool found = RecGetRecordIntOrZero("proxy.config.body_factory.enable_customizations", &enabled) == REC_ERR_OKAY;

We can change RecGetRecordIntOrZero to return the config value and take a pointer to RecErrT instead.

RecInt RecGetRecordIntOrZero3(const char *name, RecErrT *err = nullptr, bool lock = true);
RecInt
RecGetRecordIntOrZero2(const char *name, RecErrT *err, bool lock)
{
  RecData data;
  RecErrT rec_err = RecGetRecord_Xmalloc(name, RECD_INT, &data, lock);
  if (err) {
    *err = rec_err;
  }
  return rec_err == REC_ERR_OKAY ? data.rec_int : 0;
}

However we need one more line after rewriting to use this version.

RecErrT rec_err;
int     enabled = RecGetRecordIntOrZero2("proxy.config.body_factory.enable_customizations", &rec_err);
bool    found   = rec_err == REC_ERR_OKAY;

Maybe we can take a pointer to bool instead of RecErrT.

RecInt RecGetRecordIntOrZero3(const char *name, bool *found = nullptr, bool lock = true);
RecInt
RecGetRecordIntOrZero3(const char *name, bool *found, bool lock)
{
  RecData data;
  RecErrT rec_err = RecGetRecord_Xmalloc(name, RECD_INT, &data, lock);
  if (found) {
    *found = rec_err == REC_ERR_OKAY;
  }
  return rec_err == REC_ERR_OKAY ? data.rec_int : 0;
}
bool found;
int  enabled = RecGetRecordIntOrZero2("proxy.config.body_factory.enable_customizations", &found);

I welcome other suggestions if any.

@hnakamur
Copy link
Contributor Author

Another candidate would be returning std::pair.
For example, return std::pair<RecFloat, RecErrT> from RecGetRecordFloat like hnakamur@1320061

@hnakamur
Copy link
Contributor Author

hnakamur commented Mar 10, 2025

Also, how about change the type of the name argument from const char * to const std::string_view &?

std::pair<RecFloat, RecErrT> RecGetRecordFloat(const std::string_view &name, bool lock = true);

@cmcfarlen cmcfarlen modified the milestones: 10.1.0, 10.2.0 Mar 10, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eliminate deprecated lib records APIs
3 participants