[ovs-dev] [PATCH v2 1/1] netdev-dpdk: Fix egress policer error detection bug.

[Daniele Di Proietto]

2016-08-09 10:20 GMT-07:00 Ian Stokes <ian.stokes at intel.com>:

> When egress policer is set as a QoS type for a port, an error may occur
> during
> setup if incorrect parameters are used for the rte_meter. If this occurs
> the egress policer construct and set functions should free any allocated
> memory relevant to the policer and set the QoS configuration pointer to
> null. The netdev_dpdk_set_qos function should check the error value
> returned
> for any QoS construct/set calls with an assertion to avoid segfault.
> Also this commit modifies egress_policer_qos_set() to correctly lock the
> QoS
> spinlock while the egress policer configuration is updated to avoid
> segfault.
>
> Signed-off-by: Ian Stokes <ian.stokes at intel.com>
> ---
> v2
> * netdev-dpdk.c
> - Simplify assertion in netdev_dpdk_set_qos() to check that no error
>   has been returned and that a QoS configuration exists before checking
>   and logging an error.
> - Use rte_strerror  in netdev_dpdk_set_qos() when logging error for a
>   textual representation.
> - Align VLOG message for correct formatting in netdev_dpdk_set_qos().
> - egress_policer_qos_construct() now returns positive error.
> - egress_policer_qos_set() now return positive error.
> - Document addition of spinlock in egress_policer_qos_set() in commit
>   message.
> ---
>  lib/netdev-dpdk.c |   30 ++++++++++++++++++++++++++++--
>  1 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index bf3a898..f37130e 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2731,11 +2731,16 @@ netdev_dpdk_set_qos(struct netdev *netdev,
>
>              /* Install new QoS configuration. */
>              error = new_ops->qos_construct(netdev, details);
> -            ovs_assert((error == 0) == (dev->qos_conf != NULL));
>          }
>      } else {
>          error = new_ops->qos_construct(netdev, details);
> -        ovs_assert((error == 0) == (dev->qos_conf != NULL));
> +    }
> +
> +    ovs_assert((error == 0) == (dev->qos_conf != NULL));
> +    if (error) {
> +        VLOG_ERR("Failed to set QoS type %s on port %s, returned error:
> %s",
> +                 type, netdev->name, rte_strerror(-error));
> +        ovs_assert(dev->qos_conf == NULL);
>

This assert should be unnecessary, given the assert above the if.

I removed it and I pushed this to master, thanks