[ovs-dev,patch_v1] dpif: Fix cleanup of userspace datapath.

Hardware offload introduced extra tracking of netdev ports.  This
included ovs-netdev, which is really for internal infra usage for
the userpace datapath.  This breaks cleanup of the userspace
datapath.  There is no need to do this extra tracking of this port,
hence it is skipped by checking for the port name as the type does
not help here.  Adding an extra type or field is another way to fix
this, but this would probably be overkill as this port is a constant
and the misuse potential very limited.

CC: Paul Blakey <paulb@mellanox.com>
Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
Signed-off-by: Darrell Ball <dlu998@gmail.com>
---
 lib/dpif.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

On Thu, Jun 22, 2017 at 09:27:11PM -0700, Darrell Ball wrote:
> Hardware offload introduced extra tracking of netdev ports.  This
> included ovs-netdev, which is really for internal infra usage for
> the userpace datapath.  This breaks cleanup of the userspace
> datapath.  There is no need to do this extra tracking of this port,
> hence it is skipped by checking for the port name as the type does
> not help here.  Adding an extra type or field is another way to fix
> this, but this would probably be overkill as this port is a constant
> and the misuse potential very limited.
> 
> CC: Paul Blakey <paulb@mellanox.com>
> Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> ---
>  lib/dpif.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 10bdd70..8624d34 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -350,7 +350,11 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
>              struct netdev *netdev;
>              int err;
>  
> -            if (!strcmp(dpif_port.type, "internal")) {
> +            /* ovs-netdev is a tap device that is used as an
> +             * internal port for the userspace datapath, hence
> +             * don't track it here. */
> +            if (!strcmp(dpif_port.type, "internal") ||
> +                (!strcmp(dpif_port.name, "ovs-netdev"))) {

I don't understand this issue, so this is not a review, but please do
note this style point from coding-style.rst:

Do not parenthesize the operands of ``&&`` and ``||`` unless operator
precedence makes it necessary, or unless the operands are themselves
expressions that use ``&&`` and ``||``. Thus:

::

    if (!isdigit((unsigned char)s[0])
        || !isdigit((unsigned char)s[1])
        || !isdigit((unsigned char)s[2])) {
        printf("string %s does not start with 3-digit code\n", s);
    }

but

::

    if (rule && (!best || rule->priority > best->priority)) {
        best = rule;
    }

On 6/23/17, 1:26 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:

    On Thu, Jun 22, 2017 at 09:27:11PM -0700, Darrell Ball wrote:
    > Hardware offload introduced extra tracking of netdev ports.  This
    > included ovs-netdev, which is really for internal infra usage for
    > the userpace datapath.  This breaks cleanup of the userspace
    > datapath.  There is no need to do this extra tracking of this port,
    > hence it is skipped by checking for the port name as the type does
    > not help here.  Adding an extra type or field is another way to fix
    > this, but this would probably be overkill as this port is a constant
    > and the misuse potential very limited.
    > 
    > CC: Paul Blakey <paulb@mellanox.com>
    > Fixes: 32b77c316d9982("dpif: Save added ports in a port map.")
    > Signed-off-by: Darrell Ball <dlu998@gmail.com>
    > ---
    >  lib/dpif.c | 6 +++++-
    >  1 file changed, 5 insertions(+), 1 deletion(-)
    > 
    > diff --git a/lib/dpif.c b/lib/dpif.c
    > index 10bdd70..8624d34 100644
    > --- a/lib/dpif.c
    > +++ b/lib/dpif.c
    > @@ -350,7 +350,11 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
    >              struct netdev *netdev;
    >              int err;
    >  
    > -            if (!strcmp(dpif_port.type, "internal")) {
    > +            /* ovs-netdev is a tap device that is used as an
    > +             * internal port for the userspace datapath, hence
    > +             * don't track it here. */
    > +            if (!strcmp(dpif_port.type, "internal") ||
    > +                (!strcmp(dpif_port.name, "ovs-netdev"))) {
    
    I don't understand this issue, so this is not a review, but please do
    note this style point from coding-style.rst:
    
    Do not parenthesize the operands of ``&&`` and ``||`` unless operator
    precedence makes it necessary, or unless the operands are themselves
    expressions that use ``&&`` and ``||``. Thus:
    
    ::
    
        if (!isdigit((unsigned char)s[0])
            || !isdigit((unsigned char)s[1])
            || !isdigit((unsigned char)s[2])) {
            printf("string %s does not start with 3-digit code\n", s);
        }
    
    but
    
    ::
    
        if (rule && (!best || rule->priority > best->priority)) {
            best = rule;
        }

Thanks for the reminder

    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=Cvf-v52TVTcXUOH8suwH429_Yw7GH70eQv8do78OqO4&s=WfZDuxBQ0mf0fynx4BWSxBWe1OeFFvg4G_aHRCkAlCs&e=