Repository: p4lang/PI (cpp)

Issue:
Wildcard or Exact read of Meter Entry before modifying cause error
If we perform an exact read on meter entry, for example meter id = 1 and index = 1, before modifying that cell. It returns an error.
Thus, if we perform a wildcard read before modifying all the meters, it returns error.

Given the following P4RT-shell snippet:
Read before modify
```
wailok@linux:~/ONF/p4runtime-shell$ docker run -ti -v /tmp/:/tmp/ p4lang/p4runtime-sh --grpc-addr 192.168.1.125:50001 --device-id 1 --election-id 0,1 --config /tmp/p4info.txt,/tmp/bmv2.json
*** Welcome to the IPython shell for P4Runtime ***
P4Runtime sh >>> me = meter_entry["FabricIngress.bng_ingress.downstream.m_besteff"]
P4Runtime sh >>> me.index = 1
P4Runtime sh >>> me.read(lambda e: print(e))
---------------------------------------------------------------------------
P4RuntimeException                        Traceback (most recent call last)
<ipython-input-3-1d863e05a2cb> in <module>
----> 1 me.read(lambda e: print(e))
/p4runtime-sh/venv/lib/python3.8/site-packages/p4runtime_shell-0.0.1-py3.8.egg/p4runtime_sh/shell.py in read(self, function)
   1966         <self>.read(lambda c: print(c))
   1967         """
-> 1968         return super().read(function)
   1969 
   1970 
/p4runtime-sh/venv/lib/python3.8/site-packages/p4runtime_shell-0.0.1-py3.8.egg/p4runtime_sh/shell.py in read(self, function)
    722             return _EntryIterator(self, iterator)
    723         else:
--> 724             for x in _EntryIterator(self, iterator):
    725                 function(x)
    726 
/p4runtime-sh/venv/lib/python3.8/site-packages/p4runtime_shell-0.0.1-py3.8.egg/p4runtime_sh/p4runtime.py in handle(*args, **kwargs)
    132             return f(*args, **kwargs)
    133         except grpc.RpcError as e:
--> 134             raise P4RuntimeException(e) from None
    135     return handle
    136 
P4RuntimeException: P4Runtime RPC error (UNKNOWN): Error when reading meter spec from target
P4Runtime sh >>> me.pir = 100
P4Runtime sh >>> me.cir = 100
P4Runtime sh >>> me.modify()
P4Runtime sh >>> me.read(lambda e: print(e))
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 1
}
config {
  cir: 100
  pir: 100
}
```
Modifying all 8192 meters (The size is set to 8192):
```
P4Runtime sh >>> me = meter_entry["FabricIngress.bng_ingress.downstream.m_besteff"]
P4Runtime sh >>> me.read(lambda e: print(e))
---------------------------------------------------------------------------
P4RuntimeException                        Traceback (most recent call last)
<ipython-input-3-1d863e05a2cb> in <module>
----> 1 me.read(lambda e: print(e))
/p4runtime-sh/venv/lib/python3.8/site-packages/p4runtime_shell-0.0.1-py3.8.egg/p4runtime_sh/shell.py in read(self, function)
   1966         <self>.read(lambda c: print(c))
   1967         """
-> 1968         return super().read(function)
   1969 
   1970 
/p4runtime-sh/venv/lib/python3.8/site-packages/p4runtime_shell-0.0.1-py3.8.egg/p4runtime_sh/shell.py in read(self, function)
    722             return _EntryIterator(self, iterator)
    723         else:
--> 724             for x in _EntryIterator(self, iterator):
    725                 function(x)
    726 
/p4runtime-sh/venv/lib/python3.8/site-packages/p4runtime_shell-0.0.1-py3.8.egg/p4runtime_sh/p4runtime.py in handle(*args, **kwargs)
    132             return f(*args, **kwargs)
    133         except grpc.RpcError as e:
--> 134             raise P4RuntimeException(e) from None
    135     return handle
    136 
P4RuntimeException: P4Runtime RPC error (UNKNOWN): Error when reading meter spec from target
P4Runtime sh >>> for i in range(0,8192):
            ...:     me.index = i
            ...:     me.cir = 100 + i
            ...:     me.pir = 200 + i
            ...:     me.modify()
            ...: 
P4Runtime sh >>>
```
Result of wildcard read after modifying 8192 meters
```
P4Runtime sh >>> me = meter_entry["FabricIngress.bng_ingress.downstream.m_besteff"]
P4Runtime sh >>> me.read(lambda e: print(e))
... skip 8000+ result ...
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 8183
}
config {
  cir: 8283
  pir: 8383
}
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 8184
}
config {
  cir: 8284
  pir: 8384
}
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 8185
}
config {
  cir: 8285
  pir: 8385
}
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 8186
}
config {
  cir: 8286
  pir: 8386
}
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 8187
}
config {
  cir: 8287
  pir: 8387
}
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 8188
}
config {
  cir: 8288
  pir: 8388
}
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 8189
}
config {
  cir: 8289
  pir: 8389
}
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 8190
}
config {
  cir: 8290
  pir: 8390
}
meter_id: 337077280 ("FabricIngress.bng_ingress.downstream.m_besteff")
index {
  index: 8191
}
config {
  cir: 8291
  pir: 8391
}
P4Runtime sh >>>
```
[bmv2json_p4info.zip](https://github.com/p4lang/PI/files/6742315/bmv2json_p4info.zip)


Pull request:
Conform to P4Runtime specification for meter operations
* DELETE operations are invalid
* should be possible to read a meter which has not been configured (the
  "MeterConfig" field should be unset)
* if the "MeterConfig" field is unset when configuring a meter entry,
  all packets should be "green" (same as unconfigured meter)

Fixes #556

Patch:
diff --git a/proto/frontend/src/device_mgr.cpp b/proto/frontend/src/device_mgr.cpp
index 726831a..08be301 100644
--- a/proto/frontend/src/device_mgr.cpp
+++ b/proto/frontend/src/device_mgr.cpp
@@ -723,21 +723,14 @@ class DeviceMgrImp {
                             "INSERT update type not supported for meters");
       case p4v1::Update::MODIFY:
         {
-          RETURN_IF_ERROR(validate_meter_spec(meter_entry.config()));
-          auto pi_meter_spec = meter_spec_proto_to_pi(
-              meter_entry.config(), meter_entry.meter_id());
-          auto pi_status = pi_meter_set(session.get(), device_tgt,
-                                        meter_entry.meter_id(),
-                                        index,
-                                        &pi_meter_spec);
-          if (pi_status != PI_STATUS_SUCCESS)
-            RETURN_ERROR_STATUS(Code::UNKNOWN, "Error when writing meter spec");
-        }
-        break;
-      case p4v1::Update::DELETE:  // TODO(antonin): return error instead?
-        {
-          pi_meter_spec_t pi_meter_spec =
-              {0, 0, 0, 0, PI_METER_UNIT_DEFAULT, PI_METER_TYPE_DEFAULT};
+          pi_meter_spec_t pi_meter_spec;
+          if (meter_entry.has_config()) {
+            RETURN_IF_ERROR(validate_meter_spec(meter_entry.config()));
+            pi_meter_spec = meter_spec_proto_to_pi(
+                meter_entry.config(), meter_entry.meter_id());
+          } else {
+            pi_meter_spec = meter_spec_default(meter_entry.meter_id());
+          }
           auto pi_status = pi_meter_set(session.get(), device_tgt,
                                         meter_entry.meter_id(),
                                         index,
@@ -746,6 +739,9 @@ class DeviceMgrImp {
             RETURN_ERROR_STATUS(Code::UNKNOWN, "Error when writing meter spec");
         }
         break;
+      case p4v1::Update::DELETE:
+        RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT,
+                            "DELETE update type not supported for meters");
       default:
         RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT, "Invalid update type");
     }
@@ -796,23 +792,14 @@ class DeviceMgrImp {
                             "INSERT update type not supported for meters");
       case p4v1::Update::MODIFY:
         {
-          RETURN_IF_ERROR(validate_meter_spec(meter_entry.config()));
-          auto pi_meter_spec = meter_spec_proto_to_pi(
-              meter_entry.config(), table_direct_meter_id);
-          auto pi_status = pi_meter_set_direct(session.get(), device_tgt,
-                                               table_direct_meter_id,
-                                               entry_handle,
-                                               &pi_meter_spec);
-          if (pi_status != PI_STATUS_SUCCESS) {
-            RETURN_ERROR_STATUS(Code::UNKNOWN,
-                                "Error when writing direct meter spec");
+          pi_meter_spec_t pi_meter_spec;
+          if (meter_entry.has_config()) {
+            RETURN_IF_ERROR(validate_meter_spec(meter_entry.config()));
+            pi_meter_spec = meter_spec_proto_to_pi(
+                meter_entry.config(), table_direct_meter_id);
+          } else {
+            pi_meter_spec = meter_spec_default(table_direct_meter_id);
           }
-        }
-        break;
-      case p4v1::Update::DELETE:  // TODO(antonin): return error instead?
-        {
-          pi_meter_spec_t pi_meter_spec =
-              {0, 0, 0, 0, PI_METER_UNIT_DEFAULT, PI_METER_TYPE_DEFAULT};
           auto pi_status = pi_meter_set_direct(session.get(), device_tgt,
                                                table_direct_meter_id,
                                                entry_handle,
@@ -823,6 +810,9 @@ class DeviceMgrImp {
           }
         }
         break;
+      case p4v1::Update::DELETE:
+        RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT,
+                            "DELETE update type not supported for meters");
       default:
         RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT, "Invalid update type");
     }
@@ -908,7 +898,8 @@ class DeviceMgrImp {
       }
       auto entry = response->add_entities()->mutable_direct_meter_entry();
       entry->mutable_table_entry()->CopyFrom(table_entry);
-      meter_spec_pi_to_proto(meter_spec, entry->mutable_config());
+      if (!meter_spec_is_default(meter_spec))
+        meter_spec_pi_to_proto(meter_spec, entry->mutable_config());
       RETURN_OK_STATUS();
     }
 
@@ -938,14 +929,18 @@ class DeviceMgrImp {
             Code::INTERNAL,
             "Did not expect no direct resource for table entry");
       }
+      bool meter_found = false;
       for (size_t j = 0; j < direct_configs->num_configs; j++) {
         const auto &config = direct_configs->configs[j];
         if (config.res_id != table_direct_meter_id) continue;
-        meter_spec_pi_to_proto(
-            *static_cast<pi_meter_spec_t *>(config.config),
-            entry->mutable_config());
+        meter_found = true;
+        auto meter_spec = static_cast<pi_meter_spec_t *>(config.config);
+        if (!meter_spec_is_default(*meter_spec)) {
+          meter_spec_pi_to_proto(*meter_spec, entry->mutable_config());
+        }
+        break;
       }
-      if (!entry->has_config()) {
+      if (!meter_found) {
         RETURN_ERROR_STATUS(
             Code::INTERNAL,
             "Did not expect no direct meter for table entry");
@@ -1162,13 +1157,10 @@ class DeviceMgrImp {
                 entry->mutable_counter_data());
           }
         } else if (pi_is_direct_meter_id(config.res_id)) {
-          // TODO(antonin): according to the P4Runtime spec, we are not supposed
-          // to to set meter_config if the meter is in its default configuration
-          // (all packets green).
-          if (requested_entry.has_meter_config()) {
-            meter_spec_pi_to_proto(
-                *static_cast<pi_meter_spec_t *>(config.config),
-                entry->mutable_meter_config());
+          auto meter_spec = static_cast<pi_meter_spec_t *>(config.config);
+          if (requested_entry.has_meter_config() &&
+              !meter_spec_is_default(*meter_spec)) {
+            meter_spec_pi_to_proto(*meter_spec, entry->mutable_meter_config());
           }
         } else {
           RETURN_ERROR_STATUS(Code::INTERNAL, "Unknown direct resource type");
@@ -2602,8 +2594,12 @@ class DeviceMgrImp {
                             "Table has no direct meters");
       }
       RETURN_IF_ERROR(validate_meter_spec(table_entry.meter_config()));
-      *meter_spec = meter_spec_proto_to_pi(
-          table_entry.meter_config(), meter_id);
+      if (table_entry.has_meter_config()) {
+        *meter_spec = meter_spec_proto_to_pi(
+            table_entry.meter_config(), meter_id);
+      } else {
+        *meter_spec = meter_spec_default(meter_id);
+      }
       action_entry->add_direct_res_config(meter_id, meter_spec);
     }
 
@@ -2899,15 +2895,18 @@ class DeviceMgrImp {
   }
 
   static Status validate_meter_spec(const p4v1::MeterConfig &config) {
-    // as per P4Runtime spec, -1 is a valid value and means that the packet is
-    // always marked "green"
-    if (config.cir() < 0 && config.cir() != -1)
+    // The P4Runtime spec does not say anything about -1 being a valid value,
+    // but to preserve backwards-compatibility with the old implementation, we
+    // accept it. It means that the packet is always marked "green". The
+    // standard P4Runtime way of achieving the same result is to keep the
+    // appropriate MeterConfig field empty / unset.
+    if (config.cir() < -1)
       RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT, "Negative meter CIR");
-    if (config.cburst() < 0 && config.cburst() != -1)
+    if (config.cburst() < -1)
       RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT, "Negative meter CBurst");
-    if (config.pir() < 0 && config.cir() != -1)
+    if (config.pir() < -1)
       RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT, "Negative meter PIR");
-    if (config.pburst() < 0 && config.pburst() != -1)
+    if (config.pburst() < -1)
       RETURN_ERROR_STATUS(Code::INVALID_ARGUMENT, "Negative meter PBurst");
     auto max_burst = std::numeric_limits<uint32_t>::max();
     if (config.cburst() > static_cast<decltype(config.cburst())>(max_burst))
@@ -2917,6 +2916,26 @@ class DeviceMgrImp {
     RETURN_OK_STATUS();
   }
 
+  pi_meter_spec_t meter_spec_default(pi_p4_id_t meter_id) const {
+    pi_meter_spec_t pi_meter_spec;
+    pi_meter_spec.cir = static_cast<uint64_t>(-1);
+    pi_meter_spec.cburst = static_cast<uint32_t>(-1);
+    pi_meter_spec.pir = static_cast<uint64_t>(-1);
+    pi_meter_spec.pburst = static_cast<uint32_t>(-1);
+    pi_meter_spec.meter_unit =
+        (pi_meter_unit_t)pi_p4info_meter_get_unit(p4info.get(), meter_id);
+    pi_meter_spec.meter_type =
+        (pi_meter_type_t)pi_p4info_meter_get_type(p4info.get(), meter_id);
+    return pi_meter_spec;
+  }
+
+  bool meter_spec_is_default(const pi_meter_spec_t &pi_meter_spec) const {
+    return pi_meter_spec.cir == static_cast<uint64_t>(-1) &&
+        pi_meter_spec.cburst == static_cast<uint32_t>(-1) &&
+        pi_meter_spec.pir == static_cast<uint64_t>(-1) &&
+        pi_meter_spec.pburst == static_cast<uint32_t>(-1);
+  }
+
   pi_meter_spec_t meter_spec_proto_to_pi(const p4v1::MeterConfig &config,
                                          pi_p4_id_t meter_id) const {
     pi_meter_spec_t pi_meter_spec;
@@ -2978,7 +2997,9 @@ class DeviceMgrImp {
       RETURN_ERROR_STATUS(Code::UNKNOWN,
                           "Error when reading meter spec from target");
     }
-    meter_spec_pi_to_proto(meter_spec, entry->mutable_config());
+    if (!meter_spec_is_default(meter_spec)) {
+      meter_spec_pi_to_proto(meter_spec, entry->mutable_config());
+    }
     RETURN_OK_STATUS();
   }
 


Test patch:
diff --git a/proto/tests/matchers.cpp b/proto/tests/matchers.cpp
index ce71166..2d1392e 100644
--- a/proto/tests/matchers.cpp
+++ b/proto/tests/matchers.cpp
@@ -184,41 +184,46 @@ ActionDataMatcher::DescribeNegationTo(std::ostream *os) const {
 
 MeterSpecMatcher::MeterSpecMatcher(const MeterConfig &config,
                                    pi_meter_unit_t meter_unit,
-                                   pi_meter_type_t meter_type)
-    : config(config), meter_unit(meter_unit), meter_type(meter_type) { }
+                                   pi_meter_type_t meter_type) {
+  expected_spec.meter_unit = meter_unit;
+  expected_spec.meter_type = meter_type;
+  expected_spec.cir = static_cast<uint64_t>(config.cir());
+  expected_spec.cburst = static_cast<uint32_t>(config.cburst());
+  expected_spec.pir = static_cast<uint64_t>(config.pir());
+  expected_spec.pburst = static_cast<uint32_t>(config.pburst());
+}
+
+MeterSpecMatcher::MeterSpecMatcher(const pi_meter_spec_t &spec)
+    : expected_spec(spec) { }
 
 bool
 MeterSpecMatcher::MatchAndExplain(const pi_meter_spec_t *spec,
                                   MatchResultListener *listener) const {
-  auto cir = static_cast<uint64_t>(config.cir());
-  if (spec->cir != cir) {
-    *listener << "Invalid CIR (expected " << cir << " but got "
-              << spec->cir << ")";
+  if (spec->cir != expected_spec.cir) {
+    *listener << "Invalid CIR (expected " << expected_spec.cir
+              << " but got " << spec->cir << ")";
     return false;
   }
-  auto cburst = static_cast<uint32_t>(config.cburst());
-  if (spec->cburst != cburst) {
-    *listener << "Invalid CBurst (expected " << cburst << " but got "
-              << spec->cburst << ")";
+  if (spec->cburst != expected_spec.cburst) {
+    *listener << "Invalid CBurst (expected " << expected_spec.cburst
+              << " but got " << spec->cburst << ")";
     return false;
   }
-  auto pir = static_cast<uint64_t>(config.pir());
-  if (spec->pir != pir) {
-    *listener << "Invalid PIR (expected " << pir << " but got "
-              << spec->pir << ")";
+  if (spec->pir != expected_spec.pir) {
+    *listener << "Invalid PIR (expected " << expected_spec.pir
+              << " but got " << spec->pir << ")";
     return false;
   }
-  auto pburst = static_cast<uint32_t>(config.pburst());
-  if (spec->pburst != pburst) {
-    *listener << "Invalid Pburst (expected " << pburst << " but got "
-              << spec->pburst << ")";
+  if (spec->pburst != expected_spec.pburst) {
+    *listener << "Invalid Pburst (expected " << expected_spec.pburst
+              << " but got " << spec->pburst << ")";
     return false;
   }
-  if (spec->meter_unit != meter_unit) {
+  if (spec->meter_unit != expected_spec.meter_unit) {
     *listener << "Invalid meter unit";
     return false;
   }
-  if (spec->meter_type != meter_type) {
+  if (spec->meter_type != expected_spec.meter_type) {
     *listener << "Invalid meter type";
     return false;
   }
diff --git a/proto/tests/matchers.h b/proto/tests/matchers.h
index 6ab552b..c1a3cc4 100644
--- a/proto/tests/matchers.h
+++ b/proto/tests/matchers.h
@@ -165,6 +165,8 @@ class MeterSpecMatcher : public MatcherInterface<const pi_meter_spec_t *> {
                    pi_meter_unit_t meter_unit,
                    pi_meter_type_t meter_type);
 
+  explicit MeterSpecMatcher(const pi_meter_spec_t &spec);
+
   bool MatchAndExplain(const pi_meter_spec_t *spec,
                        MatchResultListener *listener) const override;
 
@@ -173,9 +175,7 @@ class MeterSpecMatcher : public MatcherInterface<const pi_meter_spec_t *> {
   void DescribeNegationTo(std::ostream *os) const override;
 
  private:
-  p4::v1::MeterConfig config;
-  pi_meter_unit_t meter_unit;
-  pi_meter_type_t meter_type;
+  pi_meter_spec_t expected_spec;
 };
 
 inline Matcher<const pi_meter_spec_t *> CorrectMeterSpec(
@@ -184,6 +184,11 @@ inline Matcher<const pi_meter_spec_t *> CorrectMeterSpec(
   return MakeMatcher(new MeterSpecMatcher(config, meter_unit, meter_type));
 }
 
+inline Matcher<const pi_meter_spec_t *> CorrectMeterSpec(
+    const pi_meter_spec_t &spec) {
+  return MakeMatcher(new MeterSpecMatcher(spec));
+}
+
 class CounterDataMatcher : public MatcherInterface<const pi_counter_data_t *> {
  public:
   CounterDataMatcher(const p4::v1::CounterData &data,
diff --git a/proto/tests/test_proto_fe.cpp b/proto/tests/test_proto_fe.cpp
index d3f4be0..db9355a 100644
--- a/proto/tests/test_proto_fe.cpp
+++ b/proto/tests/test_proto_fe.cpp
@@ -2670,6 +2670,40 @@ TEST_F(IndirectMeterTest, WriteAndRead) {
   EXPECT_PROTO_EQ(read_meter_entry, meter_entry);
 }
 
+TEST_F(IndirectMeterTest, WriteAndReadDefault) {
+  int index = 66;
+  p4v1::ReadResponse response;
+  p4v1::MeterEntry meter_entry;
+  meter_entry.set_meter_id(m_id);
+  set_index(&meter_entry, index);
+  // meter type & unit as per the P4 program
+  // default rates when no config provided (always "green")
+  pi_meter_spec_t pi_meter_spec = {
+    static_cast<uint64_t>(-1),
+    static_cast<uint32_t>(-1),
+    static_cast<uint64_t>(-1),
+    static_cast<uint32_t>(-1),
+    PI_METER_UNIT_PACKETS,
+    PI_METER_TYPE_COLOR_UNAWARE
+  };
+  auto meter_matcher = CorrectMeterSpec(pi_meter_spec);
+  EXPECT_CALL(*mock, meter_set(m_id, index, meter_matcher));
+  {
+    auto status = write_meter(&meter_entry);
+    ASSERT_EQ(status.code(), Code::OK);
+  }
+
+  EXPECT_CALL(*mock, meter_read(m_id, index, _));
+  {
+    auto status = read_meter(&meter_entry, &response);
+    ASSERT_EQ(status.code(), Code::OK);
+  }
+  const auto &entities = response.entities();
+  ASSERT_EQ(1, entities.size());
+  const auto &read_meter_entry = entities.Get(0).meter_entry();
+  EXPECT_FALSE(read_meter_entry.has_config());
+}
+
 class DirectCounterTest : public ExactOneTest {
  protected:
   DirectCounterTest()