On 20 Jun 2023, at 16:17, Aaron Conole wrote:
> Eelco Chaudron <echau...@redhat.com> writes:
>
>> Add additional error coverage counters for dpif operation failures.
>> This could help to quickly identify netlink problems when communicating
>> with the OVS kernel module.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2070630
>> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>> ---
>
> One thing that made this difficult to review is the re-ordering of the
> COVERAGE_DEFINE()s. Maybe it would be better to have a separate patch
> that corrects the order to alphabetical, and then a patch that adds the
> new counters for flow put/del/get/execute. WDYT?
Based on the patch size, I decided not to do this, but if others also feel it
needs to be split up, I can do this.
//Eelco
>> lib/dpif.c | 39 ++++++++++++++++++++++++++-------------
>> 1 file changed, 26 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 3305401fe..b1cbf39c4 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -55,18 +55,22 @@
>> VLOG_DEFINE_THIS_MODULE(dpif);
>>
>> COVERAGE_DEFINE(dpif_destroy);
>> -COVERAGE_DEFINE(dpif_port_add);
>> -COVERAGE_DEFINE(dpif_port_del);
>> +COVERAGE_DEFINE(dpif_execute);
>> +COVERAGE_DEFINE(dpif_execute_error);
>> +COVERAGE_DEFINE(dpif_execute_with_help);
>> +COVERAGE_DEFINE(dpif_flow_del);
>> +COVERAGE_DEFINE(dpif_flow_del_error);
>> COVERAGE_DEFINE(dpif_flow_flush);
>> COVERAGE_DEFINE(dpif_flow_get);
>> +COVERAGE_DEFINE(dpif_flow_get_error);
>> COVERAGE_DEFINE(dpif_flow_put);
>> -COVERAGE_DEFINE(dpif_flow_del);
>> -COVERAGE_DEFINE(dpif_execute);
>> -COVERAGE_DEFINE(dpif_purge);
>> -COVERAGE_DEFINE(dpif_execute_with_help);
>> -COVERAGE_DEFINE(dpif_meter_set);
>> -COVERAGE_DEFINE(dpif_meter_get);
>> +COVERAGE_DEFINE(dpif_flow_put_error);
>> COVERAGE_DEFINE(dpif_meter_del);
>> +COVERAGE_DEFINE(dpif_meter_get);
>> +COVERAGE_DEFINE(dpif_meter_set);
>> +COVERAGE_DEFINE(dpif_port_add);
>> +COVERAGE_DEFINE(dpif_port_del);
>> +COVERAGE_DEFINE(dpif_purge);
>>
>> static const struct dpif_class *base_dpif_classes[] = {
>> #if defined(__linux__) || defined(_WIN32)
>> @@ -1381,8 +1385,11 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops,
>> size_t n_ops,
>>
>> COVERAGE_INC(dpif_flow_put);
>> log_flow_put_message(dpif, &this_module, put, error);
>> - if (error && put->stats) {
>> - memset(put->stats, 0, sizeof *put->stats);
>> + if (error) {
>> + COVERAGE_INC(dpif_flow_put_error);
>> + if (put->stats) {
>> + memset(put->stats, 0, sizeof *put->stats);
>> + }
>> }
>> break;
>> }
>> @@ -1392,10 +1399,10 @@ dpif_operate(struct dpif *dpif, struct dpif_op
>> **ops, size_t n_ops,
>>
>> COVERAGE_INC(dpif_flow_get);
>> if (error) {
>> + COVERAGE_INC(dpif_flow_get_error);
>> memset(get->flow, 0, sizeof *get->flow);
>> }
>> log_flow_get_message(dpif, &this_module, get, error);
>> -
>> break;
>> }
>>
>> @@ -1404,8 +1411,11 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops,
>> size_t n_ops,
>>
>> COVERAGE_INC(dpif_flow_del);
>> log_flow_del_message(dpif, &this_module, del, error);
>> - if (error && del->stats) {
>> - memset(del->stats, 0, sizeof *del->stats);
>> + if (error) {
>> + COVERAGE_INC(dpif_flow_del_error);
>> + if (del->stats) {
>> + memset(del->stats, 0, sizeof *del->stats);
>> + }
>> }
>> break;
>> }
>> @@ -1414,6 +1424,9 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops,
>> size_t n_ops,
>> COVERAGE_INC(dpif_execute);
>> log_execute_message(dpif, &this_module, &op->execute,
>> false, error);
>> + if (error) {
>> + COVERAGE_INC(dpif_execute_error);
>> + }
>> break;
>> }
>> }
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
