From c478b385411f1c2dc68454e4a653c49e648bbf8b Mon Sep 17 00:00:00 2001 From: bibhuprasad-hcl <161687009+bibhuprasad-hcl@users.noreply.github.com> Date: Sun, 30 Jun 2024 21:07:27 -0400 Subject: [PATCH] [Comb] Add parsers for @refers_to and @referenced_by Return empty list instead of KNotFound when annotation does not exist.[pdpi] Refactor code base to use annotation library for refers_to annotation. (#277) * cleanup some header files.Internal Code Change.Don't craft packets with UDP headers if UDP port is already reserved. * [SFE-P4][Garbage Collection] Support multicast entity as input to garbage collection.Improve error messages of InstallPdTableEntr{y, ies}.[pdpi] Convert referring optionals to exacts in test program.[pdpi] Add IrTableEntryReference to P4info.[pdpi] Add conversions from/to string for IrBuiltIns * [pdpi] Add parsers for @refers_to and @referenced_by Return empty list instead of KNotFound when annotation does not exist.[pdpi] Refactor code base to use annotation library for refers_to annotation. --------- Co-authored-by: rhalstea Co-authored-by: Srikishen Pondicherry Shanmugam --- p4_pdpi/BUILD.bazel | 32 ++++++++++ p4_pdpi/ir.cc | 42 ++++++------- p4_pdpi/reference_annotations.cc | 52 +++++++++++++++++ p4_pdpi/reference_annotations.h | 42 +++++++++++++ p4_pdpi/reference_annotations_test.cc | 65 +++++++++++++++++++++ p4_pdpi/utils/BUILD.bazel | 2 +- p4_pdpi/utils/annotation_parser.cc | 6 +- p4_pdpi/utils/annotation_parser.h | 11 ++-- p4_pdpi/utils/annotation_parser_test.cc | 78 ++++++++----------------- p4rt_app/sonic/hashing.cc | 2 +- 10 files changed, 243 insertions(+), 89 deletions(-) create mode 100644 p4_pdpi/reference_annotations.cc create mode 100644 p4_pdpi/reference_annotations.h create mode 100644 p4_pdpi/reference_annotations_test.cc diff --git a/p4_pdpi/BUILD.bazel b/p4_pdpi/BUILD.bazel index 6a25fc8f..2e9e9f06 100644 --- a/p4_pdpi/BUILD.bazel +++ b/p4_pdpi/BUILD.bazel @@ -47,6 +47,37 @@ cc_test( ], ) +cc_library( + name = "reference_annotations", + srcs = [ + "reference_annotations.cc", + ], + hdrs = [ + "reference_annotations.h", + ], + deps = [ + "//gutil:status", + "//p4_pdpi/utils:annotation_parser", + "@com_google_absl//absl/status:statusor", + "@com_google_protobuf//:protobuf", + ], +) + +cc_test( + name = "reference_annotations_test", + srcs = [ + "reference_annotations_test.cc", + ], + deps = [ + ":reference_annotations", + "//gutil:status_matchers", + "@com_google_absl//absl/status", + "@com_google_absl//absl/strings", + "@com_google_googletest//:gtest_main", + "@com_google_protobuf//:protobuf", + ], +) + cc_library( name = "sequencing_util", srcs = ["sequencing_util.cc"], @@ -221,6 +252,7 @@ cc_library( local_defines = ["PDPI_DISABLE_TRANSLATION_OPTIONS_DEFAULT"], deps = [ ":ir_cc_proto", + ":reference_annotations", ":translation_options", "//gutil:collections", "//gutil:proto", diff --git a/p4_pdpi/ir.cc b/p4_pdpi/ir.cc index ce5ef434..cb0bd664 100644 --- a/p4_pdpi/ir.cc +++ b/p4_pdpi/ir.cc @@ -44,6 +44,7 @@ #include "p4/config/v1/p4types.pb.h" #include "p4/v1/p4runtime.pb.h" #include "p4_pdpi/ir.pb.h" +#include "p4_pdpi/reference_annotations.h" #include "p4_pdpi/translation_options.h" #include "p4_pdpi/utils/ir.h" @@ -62,6 +63,7 @@ using ::pdpi::IrActionInvocation; using ::pdpi::IrMatchFieldDefinition; using ::pdpi::IrP4Info; using ::pdpi::IrTableDefinition; +using ::pdpi::ParsedRefersToAnnotation; namespace { @@ -228,35 +230,27 @@ absl::StatusOr MatchFieldNameToId( absl::StatusOr> GetRefersToAnnotations( const p4::config::v1::P4Info &p4info, const ::google::protobuf::RepeatedPtrField &annotations) { - constexpr absl::string_view kError = "Found invalid @refers_to annotation: "; std::vector result; - for (absl::string_view annotation_contents : annotations) { - if (absl::ConsumePrefix(&annotation_contents, "@refers_to(")) { - if (!absl::ConsumeSuffix(&annotation_contents, ")")) { - return gutil::InvalidArgumentErrorBuilder() << kError << "Missing )"; - } - std::vector parts = absl::StrSplit(annotation_contents, ','); - if (parts.size() != 2) { - return gutil::InvalidArgumentErrorBuilder() - << kError << "Incorrect number of arguments, required 2 but got " - << parts.size() << " instead."; - } + ASSIGN_OR_RETURN( + const std::vector refers_to_annotations, + ParseAllRefersToAnnotations(annotations)); - absl::string_view table = absl::StripAsciiWhitespace(parts[0]); - absl::string_view match_field = absl::StripAsciiWhitespace(parts[1]); + for (const auto &refers_to_annotation : refers_to_annotations) { + absl::string_view table = refers_to_annotation.table; + absl::string_view match_field = refers_to_annotation.field; - ASSIGN_OR_RETURN(uint32_t table_id, TableAliasToId(p4info, table)); - ASSIGN_OR_RETURN(uint32_t match_field_id, - MatchFieldNameToId(p4info, table_id, match_field)); + ASSIGN_OR_RETURN(uint32_t table_id, TableAliasToId(p4info, table)); + ASSIGN_OR_RETURN(uint32_t match_field_id, + MatchFieldNameToId(p4info, table_id, match_field)); - IrMatchFieldReference reference; - reference.set_table(std::string(table)); - reference.set_match_field(std::string(match_field)); - reference.set_table_id(table_id); - reference.set_match_field_id(match_field_id); - result.push_back(reference); - } + IrMatchFieldReference reference; + reference.set_table(std::string(table)); + reference.set_match_field(std::string(match_field)); + reference.set_table_id(table_id); + reference.set_match_field_id(match_field_id); + result.push_back(reference); } + return result; } diff --git a/p4_pdpi/reference_annotations.cc b/p4_pdpi/reference_annotations.cc new file mode 100644 index 00000000..940328ba --- /dev/null +++ b/p4_pdpi/reference_annotations.cc @@ -0,0 +1,52 @@ +#include "p4_pdpi/reference_annotations.h" + +#include +#include + +#include "absl/status/statusor.h" +#include "google/protobuf/repeated_ptr_field.h" +#include "gutil/status.h" +#include "p4_pdpi/utils/annotation_parser.h" + +namespace pdpi { + +using google::protobuf::RepeatedPtrField; + +namespace { + +absl::StatusOr ParseAsRefersToAnnotation( + std::string body) { + ASSIGN_OR_RETURN(auto arg_list, annotation::ParseAsArgList(body)); + if (arg_list.size() != 2) { + return gutil::InvalidArgumentErrorBuilder() + << "Invalid argument. Expected 2 args, but got " << arg_list.size(); + } + return ParsedRefersToAnnotation{.table = arg_list[0], .field = arg_list[1]}; +} + +absl::StatusOr ParseAsReferencedByAnnotation( + std::string body) { + ASSIGN_OR_RETURN(auto arg_list, annotation::ParseAsArgList(body)); + if (arg_list.size() != 2) { + return gutil::InvalidArgumentErrorBuilder() + << "Invalid argument. Expected 2 args, but got " << arg_list.size(); + } + return ParsedReferencedByAnnotation{.table = arg_list[0], + .field = arg_list[1]}; +} + +} // namespace + +absl::StatusOr> +ParseAllRefersToAnnotations(const RepeatedPtrField& annotations) { + return GetAllParsedAnnotations( + "refers_to", annotations, ParseAsRefersToAnnotation); +} + +absl::StatusOr> +ParseAllReferencedByAnnotations( + const RepeatedPtrField& annotations) { + return GetAllParsedAnnotations( + "referenced_by", annotations, ParseAsReferencedByAnnotation); +} +} // namespace pdpi diff --git a/p4_pdpi/reference_annotations.h b/p4_pdpi/reference_annotations.h new file mode 100644 index 00000000..8d359192 --- /dev/null +++ b/p4_pdpi/reference_annotations.h @@ -0,0 +1,42 @@ +#ifndef PINS_P4_PDPI_REFERENCE_ANNOTATIONS_H_ +#define PINS_P4_PDPI_REFERENCE_ANNOTATIONS_H_ + +#include +#include + +#include "absl/status/statusor.h" +#include "google/protobuf/repeated_ptr_field.h" + +namespace pdpi { + +// Parsed contents of an `@refers_to(table, field)` annotation. +struct ParsedRefersToAnnotation { + std::string table; + std::string field; +}; + +// Contents of an `@reference_by(table, field)` annotation. +struct ParsedReferencedByAnnotation { + std::string table; + std::string field; +}; + +// Returns a list of `RefersToAnnotation` parsed from the `annotations`. +// Returns empty list if no annotation contained the label `@refers_to`. +// Return InvalidArgument if any annotation with the label `@refers_to` did not +// contain exactly 2 arguments. +absl::StatusOr> +ParseAllRefersToAnnotations( + const google::protobuf::RepeatedPtrField& annotations); + +// Returns a list of `ReferencedByAnnotation`s parsed from the `annotations`. +// Returns empty list if no annotation contained the label `@referenced_by`. +// Return InvalidArgument if any annotation with the label `@referenced_by` did +// not contain exactly 2 arguments. +absl::StatusOr> +ParseAllReferencedByAnnotations( + const google::protobuf::RepeatedPtrField& annotations); + +} // namespace pdpi + +#endif // PINS_P4_PDPI_REFERENCE_ANNOTATIONS_H_ diff --git a/p4_pdpi/reference_annotations_test.cc b/p4_pdpi/reference_annotations_test.cc new file mode 100644 index 00000000..66b6660c --- /dev/null +++ b/p4_pdpi/reference_annotations_test.cc @@ -0,0 +1,65 @@ +#include "p4_pdpi/reference_annotations.h" + +#include + +#include "absl/status/status.h" +#include "absl/strings/substitute.h" +#include "gmock/gmock.h" +#include "google/protobuf/repeated_ptr_field.h" +#include "gtest/gtest.h" +#include "gutil/status_matchers.h" + +namespace pdpi { +namespace { + +using ::gutil::IsOkAndHolds; +using ::gutil::StatusIs; +using ::testing::ElementsAre; + +// -- Matchers ----------------------------------------------------------------- + +MATCHER_P2(HasTableAndField, table, field, + absl::Substitute("Matches: [table: $0, field: $1]", table, field)) { + return arg.table == table && arg.field == field; +} + +// -- Tests -------------------------------------------------------------------- + +TEST(ParseAllRefersToAnnotationArgs, ReturnsAllRefersToAnnotations) { + google::protobuf::RepeatedPtrField annotations; + annotations.Add("@refers_to(a,b)"); + annotations.Add("@referenced_by(c,d)"); + annotations.Add("@refers_to(\n e,f \t)"); + EXPECT_THAT(ParseAllRefersToAnnotations(annotations), + IsOkAndHolds(ElementsAre(HasTableAndField("a", "b"), + HasTableAndField("e", "f")))); +} + +TEST(ParseAllRefersToAnnotationArgs, + FailsWithRefersToWithMoreThanTwoArguments) { + google::protobuf::RepeatedPtrField annotations; + annotations.Add("@refers_to(a,b,c)"); + EXPECT_THAT(ParseAllRefersToAnnotations(annotations), + StatusIs(absl::StatusCode::kInvalidArgument)); +} + +TEST(ParseAllReferencedByAnnotationArgs, ReturnsAllRefersToAnnotations) { + google::protobuf::RepeatedPtrField annotations; + annotations.Add("@referenced_by(a,b)"); + annotations.Add("@refers_to(c,d)"); + annotations.Add("@referenced_by(\n e,f \t)"); + EXPECT_THAT(ParseAllReferencedByAnnotations(annotations), + IsOkAndHolds(ElementsAre(HasTableAndField("a", "b"), + HasTableAndField("e", "f")))); +} + +TEST(ParseAllReferencedByAnnotationArgs, + FailsWithReferencedByWithMoreThanTwoArguments) { + google::protobuf::RepeatedPtrField annotations; + annotations.Add("@referenced_by(a,b,c)"); + EXPECT_THAT(ParseAllReferencedByAnnotations(annotations), + StatusIs(absl::StatusCode::kInvalidArgument)); +} + +} // namespace +} // namespace pdpi diff --git a/p4_pdpi/utils/BUILD.bazel b/p4_pdpi/utils/BUILD.bazel index 82787d95..a2a8fe14 100644 --- a/p4_pdpi/utils/BUILD.bazel +++ b/p4_pdpi/utils/BUILD.bazel @@ -37,7 +37,7 @@ cc_test( srcs = ["annotation_parser_test.cc"], deps = [ ":annotation_parser", - "//gutil:status", + "//gutil:status_matchers", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", diff --git a/p4_pdpi/utils/annotation_parser.cc b/p4_pdpi/utils/annotation_parser.cc index c0519511..7c3340d9 100644 --- a/p4_pdpi/utils/annotation_parser.cc +++ b/p4_pdpi/utils/annotation_parser.cc @@ -26,7 +26,9 @@ namespace pdpi { namespace { -inline bool IsWhitespace(char c) { return c == ' ' || c == '\t'; } +inline bool IsWhitespace(char c) { + return c == ' ' || c == '\t' || c == '\n' || c == '\r'; +} } // namespace // Use a custom parser for improved speed and error reporting compared to a @@ -147,6 +149,8 @@ absl::StatusOr> ParseAsArgList(std::string value) { break; case '\t': case ' ': + case '\n': + case '\r': continue; case ')': case ']': diff --git a/p4_pdpi/utils/annotation_parser.h b/p4_pdpi/utils/annotation_parser.h index 018d4b48..5351e99d 100644 --- a/p4_pdpi/utils/annotation_parser.h +++ b/p4_pdpi/utils/annotation_parser.h @@ -84,7 +84,6 @@ std::vector GetAllAnnotations( } // Returns a list of the parsed body of all annotations with the given label. -// Returns a Status with code kNotFound if there is no matching annotation. // // Note: Currently, template type deduction does not work for this function. To // use GetAllParsedAnnotations, declare it with the parser's StatusOr T type. @@ -110,10 +109,6 @@ absl::StatusOr> GetAllParsedAnnotations( values.push_back(value); } } - if (values.empty()) { - return gutil::NotFoundErrorBuilder() - << "No annotation contained label \"" << label << "\""; - } return values; } @@ -136,6 +131,10 @@ absl::StatusOr GetParsedAnnotation(absl::string_view label, // precompiler from splitting GetAllParsedAnnotations( label, annotations, parser))); + if (values.empty()) { + return gutil::NotFoundErrorBuilder() + << "No annotations contained label \"" << label << "\""; + } if (values.size() > 1) { return gutil::InvalidArgumentErrorBuilder() << "Multiple annotations contained label \"" << label << "\""; @@ -155,7 +154,6 @@ absl::StatusOr> GetAnnotationAsArgList( // Returns the string list form of the body of all annotations with the given // label. -// Returns a Status with code kNotFound if there is no matching annotation. template absl::StatusOr>> GetAllAnnotationsAsArgList(absl::string_view label, @@ -173,7 +171,6 @@ absl::StatusOr GetAnnotationBody(absl::string_view label, } // Returns all annotation bodies from all annotations with the given label. -// Returns a Status with code kNotFound if there is no matching annotation. template absl::StatusOr> GetAllAnnotationBodies( absl::string_view label, const Container& annotations) { diff --git a/p4_pdpi/utils/annotation_parser_test.cc b/p4_pdpi/utils/annotation_parser_test.cc index 5199d408..6106f64a 100644 --- a/p4_pdpi/utils/annotation_parser_test.cc +++ b/p4_pdpi/utils/annotation_parser_test.cc @@ -28,13 +28,14 @@ #include "absl/strings/string_view.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -#include "gutil/status.h" +#include "gutil/status_matchers.h" namespace pdpi { namespace annotation { namespace { -using ::testing::_; +using ::gutil::IsOkAndHolds; +using ::gutil::StatusIs; using ::testing::ElementsAre; using ::testing::IsEmpty; @@ -49,32 +50,6 @@ std::set Keys(T map_container) { // === Status Matchers === -MATCHER_P2(StatusIs, code, message, "") { - auto code_matcher = testing::MatcherCast(code); - auto message_matcher = testing::MatcherCast(message); - if (testing::Matches(code_matcher)(arg.code()) && - testing::Matches(message_matcher)(arg.message())) { - return true; - } - *result_listener << "Expected status whose code matches "; - code_matcher.DescribeTo(result_listener->stream()); - *result_listener << "and whose message matches "; - message_matcher.DescribeTo(result_listener->stream()); - return false; -} - -MATCHER_P(IsOkAndHolds, value, "") { - if (!arg.ok()) { - *result_listener << "Expected status is ok. Actual status is : " - << arg.status(); - return false; - } - - auto matcher = testing::MatcherCast< - typename std::remove_reference::type::value_type>(value); - return ExplainMatchResult(matcher, arg.value(), result_listener); -} - // Mock parser to ensure parsing is not invoked. absl::StatusOr ExpectNoParsing(std::string) { ADD_FAILURE() << "Parser is not expected to be called."; @@ -89,7 +64,7 @@ MATCHER_P2(AnnotationComponentsAre, label, body, "") { testing::Matches(body_matcher)(arg.body)) { return true; } - *result_listener << "Expected AnnotationComponents whose labe matches "; + *result_listener << "Expected AnnotationComponents whose label matches "; label_matcher.DescribeTo(result_listener->stream()); *result_listener << "and whose body matches "; body_matcher.DescribeTo(result_listener->stream()); @@ -108,25 +83,22 @@ TEST(GetAllAnnotations, ReturnsLabelBodyAnnotations) { TEST(GetParsedAnnotation, EmptyAnnotationListReturnsNotFound) { std::vector empty; - EXPECT_THAT( - GetParsedAnnotation("label", empty, ExpectNoParsing).status(), - StatusIs(absl::StatusCode::kNotFound, _)); + EXPECT_THAT(GetParsedAnnotation("label", empty, ExpectNoParsing), + StatusIs(absl::StatusCode::kNotFound)); } TEST(GetParsedAnnotation, MultipleMatchingAnnotationsReturnsError) { EXPECT_THAT( GetAnnotationAsArgList( "label", std::vector( - {"@a(b)", "@b(a)", "@label(arg)", "@label(arg2)"})) - .status(), - StatusIs(absl::StatusCode::kInvalidArgument, _)); + {"@a(b)", "@b(a)", "@label(arg)", "@label(arg2)"})), + StatusIs(absl::StatusCode::kInvalidArgument)); } -TEST(GetAllParsedAnnotations, EmptyAnnotationListReturnsNotFound) { +TEST(GetAllParsedAnnotations, EmptyAnnotationListReturnsEmptyList) { std::vector empty; - EXPECT_THAT( - GetAllParsedAnnotations("label", empty, ExpectNoParsing).status(), - StatusIs(absl::StatusCode::kNotFound, _)); + EXPECT_THAT(GetAllParsedAnnotations("label", empty, ExpectNoParsing), + IsOkAndHolds(IsEmpty())); } TEST(GetAllParsedAnnotations, ReturnsAllMatchingAnnotations) { @@ -190,18 +162,16 @@ TEST_P(NonMatchingAnnotationTest, GetAnnotationReturnsNotFound) { EXPECT_THAT( GetParsedAnnotation( "label", std::vector({TestCases().at(GetParam())}), - ExpectNoParsing) - .status(), - StatusIs(absl::StatusCode::kNotFound, _)); + ExpectNoParsing), + StatusIs(absl::StatusCode::kNotFound)); } -TEST_P(NonMatchingAnnotationTest, GetAllParsedAnnotationsReturnsNotFound) { +TEST_P(NonMatchingAnnotationTest, GetAllParsedAnnotationsReturnsEmptyList) { EXPECT_THAT( GetAllParsedAnnotations( "label", std::vector({TestCases().at(GetParam())}), - ExpectNoParsing) - .status(), - StatusIs(absl::StatusCode::kNotFound, _)); + ExpectNoParsing), + IsOkAndHolds(IsEmpty())); } TEST_P(NonMatchingAnnotationTest, GetParsedAnnotationSkipsAnnotation) { @@ -230,8 +200,7 @@ TEST(GetParsedAnnotation, ReturnsParserError) { }; EXPECT_THAT( GetParsedAnnotation("label", std::vector({"@label()"}), - parser) - .status(), + parser), StatusIs(absl::StatusCode::kUnknown, testing::HasSubstr("ErrorMessage"))); } @@ -241,8 +210,7 @@ TEST(GetAllParsedAnnotations, ReturnsParserError) { }; EXPECT_THAT( GetAllParsedAnnotations( - "label", std::vector({"@label()"}), parser) - .status(), + "label", std::vector({"@label()"}), parser), StatusIs(absl::StatusCode::kUnknown, testing::HasSubstr("ErrorMessage"))); } @@ -556,14 +524,14 @@ std::string UnpairedCharacterCasesName(char c) { class UnpairedCharacterTest : public testing::TestWithParam {}; TEST_P(UnpairedCharacterTest, ReturnsInvalidArgument) { - EXPECT_THAT(ParseAsArgList(std::string(1, GetParam())).status(), - StatusIs(absl::StatusCode::kInvalidArgument, _)); + EXPECT_THAT(ParseAsArgList(std::string(1, GetParam())), + StatusIs(absl::StatusCode::kInvalidArgument)); } TEST_P(UnpairedCharacterTest, ReturnsInvalidArgumentWithinNest) { - EXPECT_THAT(ParseAsArgList(absl::StrCat("(", std::string(1, GetParam()), ")")) - .status(), - StatusIs(absl::StatusCode::kInvalidArgument, _)); + EXPECT_THAT( + ParseAsArgList(absl::StrCat("(", std::string(1, GetParam()), ")")), + StatusIs(absl::StatusCode::kInvalidArgument)); } INSTANTIATE_TEST_SUITE_P( diff --git a/p4rt_app/sonic/hashing.cc b/p4rt_app/sonic/hashing.cc index 60cb781f..3b909ab0 100644 --- a/p4rt_app/sonic/hashing.cc +++ b/p4rt_app/sonic/hashing.cc @@ -210,7 +210,7 @@ absl::StatusOr> GenerateAppDbHashFieldEntries( for (const auto& [action_name, action_def] : ir_p4info.actions_by_name()) { auto parse_results = pdpi::GetAllAnnotationsAsArgList( "sai_native_hash_field", action_def.preamble().annotations()); - if (!parse_results.ok()) continue; + if (!parse_results.ok() || parse_results->empty()) continue; auto json = GenerateJsonHashFieldEntries(*parse_results); if (!json.ok()) { return gutil::InvalidArgumentErrorBuilder()