bryancall commented on code in PR #12948:
URL: https://github.com/apache/trafficserver/pull/12948#discussion_r2898414234
##########
plugins/header_rewrite/conditions_geo_maxmind.cc:
##########
@@ -91,33 +92,36 @@ MMConditionGeo::get_geo_string(const sockaddr *addr) const
return ret;
}
- const char *field_name;
+ MMDB_entry_data_s entry_data;
+
switch (_geo_qual) {
case GEO_QUAL_COUNTRY:
- field_name = "country_code";
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "names",
"en", NULL);
+ break;
+ case GEO_QUAL_COUNTRY_ISO:
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "iso_code",
NULL);
break;
Review Comment:
Fixed — removed the dead `GEO_QUAL_COUNTRY_ISO` case from `get_geo_string()`
since it's dispatched as an int type and never reaches this code path.
##########
plugins/header_rewrite/conditions_geo_maxmind.cc:
##########
@@ -91,33 +92,36 @@ MMConditionGeo::get_geo_string(const sockaddr *addr) const
return ret;
}
- const char *field_name;
+ MMDB_entry_data_s entry_data;
+
switch (_geo_qual) {
case GEO_QUAL_COUNTRY:
- field_name = "country_code";
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "names",
"en", NULL);
Review Comment:
Good catch — changed `GEO_QUAL_COUNTRY` to use `country/iso_code` so it
returns "US", "KR", etc., matching the legacy GeoIP backend's
`GeoIP_country_code_by_ipnum` behavior.
##########
plugins/header_rewrite/conditions_geo_maxmind.cc:
##########
@@ -91,33 +92,36 @@ MMConditionGeo::get_geo_string(const sockaddr *addr) const
return ret;
}
- const char *field_name;
+ MMDB_entry_data_s entry_data;
+
switch (_geo_qual) {
case GEO_QUAL_COUNTRY:
- field_name = "country_code";
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "names",
"en", NULL);
+ break;
+ case GEO_QUAL_COUNTRY_ISO:
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "iso_code",
NULL);
break;
case GEO_QUAL_ASN_NAME:
- field_name = "autonomous_system_organization";
+ status = MMDB_get_value(&result.entry, &entry_data,
"autonomous_system_organization", NULL);
break;
default:
Dbg(pi_dbg_ctl, "Unsupported field %d", _geo_qual);
+ MMDB_free_entry_data_list(entry_data_list);
return ret;
- break;
}
- MMDB_entry_data_s entry_data;
-
- status = MMDB_get_value(&result.entry, &entry_data, field_name, NULL);
if (MMDB_SUCCESS != status) {
- Dbg(pi_dbg_ctl, "ERROR on get value asn value: %s", MMDB_strerror(status));
+ Dbg(pi_dbg_ctl, "Error looking up geo string field: %s",
MMDB_strerror(status));
+ MMDB_free_entry_data_list(entry_data_list);
return ret;
}
- ret = std::string(entry_data.utf8_string, entry_data.data_size);
- if (nullptr != entry_data_list) {
- MMDB_free_entry_data_list(entry_data_list);
+ if (entry_data.has_data && entry_data.type == MMDB_DATA_TYPE_UTF8_STRING) {
+ ret = std::string(entry_data.utf8_string, entry_data.data_size);
}
+ MMDB_free_entry_data_list(entry_data_list);
+
return ret;
Review Comment:
Fixed — removed `MMDB_get_entry_data_list()` and all related code from both
`get_geo_string()` and `get_geo_int()`. `MMDB_get_value()` operates directly on
the entry, so the data list was unnecessary overhead.
##########
plugins/header_rewrite/conditions_geo_maxmind.cc:
##########
@@ -91,33 +92,36 @@ MMConditionGeo::get_geo_string(const sockaddr *addr) const
return ret;
}
- const char *field_name;
+ MMDB_entry_data_s entry_data;
+
switch (_geo_qual) {
case GEO_QUAL_COUNTRY:
- field_name = "country_code";
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "names",
"en", NULL);
+ break;
+ case GEO_QUAL_COUNTRY_ISO:
+ status = MMDB_get_value(&result.entry, &entry_data, "country", "iso_code",
NULL);
break;
case GEO_QUAL_ASN_NAME:
- field_name = "autonomous_system_organization";
+ status = MMDB_get_value(&result.entry, &entry_data,
"autonomous_system_organization", NULL);
break;
Review Comment:
Added a unit test in `header_rewrite_test.cc` that validates the MMDB lookup
paths (`country/iso_code`, `country/names/en`) against a real database. It
skips gracefully if no MMDB file is installed, and can be pointed at a specific
file via `MMDB_COUNTRY_PATH` env var.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
