[ovs-dev] [PATCH] netdev: Free packets in netdev_send() for devices that don't support send.

[Jarno Rajahalme]

IMO we should figure out a name for the “may_steal” that carries the intended semantics a little bit better. Plain “steal” instead of “may_steal” would be better, but maybe not the best choice.

Acked-by: Jarno Rajahalme <jarno at ovn.org>

> On Feb 8, 2016, at 10:43 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> This manifested as a memory leak in test 898 "ofproto-dpif - sFlow packet
> sampling - tunnel set", which included an output to a tunnel vport that
> doesn't have an implementation of netdev_send().
> 
> Reported-by: William Tu <u9012063 at gmail.com>
> Reported-at: http://openvswitch.org/pipermail/dev/2016-February/065873.html
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
> lib/netdev-provider.h |  6 ++++--
> lib/netdev.c          | 20 ++++++++++++++------
> 2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> index d324ffc..2aa1b5d 100644
> --- a/lib/netdev-provider.h
> +++ b/lib/netdev-provider.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -309,7 +309,9 @@ struct netdev_class {
>      * If the function returns a non-zero value, some of the packets might have
>      * been sent anyway.
>      *
> -     * To retain ownership of 'buffers' caller can set may_steal to false.
> +     * If 'may_steal' is false, the caller retains ownership of all the
> +     * packets.  If 'may_steal' is true, the caller transfers ownership of all
> +     * the packets to the network device, regardless of success.
>      *
>      * The network device is expected to maintain one or more packet
>      * transmission queues, so that the caller does not ordinarily have to
> diff --git a/lib/netdev.c b/lib/netdev.c
> index c250c93..e27f854 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -728,7 +728,9 @@ netdev_set_multiq(struct netdev *netdev, unsigned int n_txq,
>  * If the function returns a non-zero value, some of the packets might have
>  * been sent anyway.
>  *
> - * To retain ownership of 'buffer' caller can set may_steal to false.
> + * If 'may_steal' is false, the caller retains ownership of all the packets.
> + * If 'may_steal' is true, the caller transfers ownership of all the packets
> + * to the network device, regardless of success.
>  *
>  * The network device is expected to maintain one or more packet
>  * transmission queues, so that the caller does not ordinarily have to
> @@ -742,11 +744,17 @@ int
> netdev_send(struct netdev *netdev, int qid, struct dp_packet **buffers,
>             int cnt, bool may_steal)
> {
> -    int error;
> +    if (!netdev->netdev_class->send) {
> +        if (may_steal) {
> +            for (int i = 0; i < cnt; i++) {
> +                dp_packet_delete(buffers[i]);
> +            }
> +        }
> +        return EOPNOTSUPP;
> +    }
> 
> -    error = (netdev->netdev_class->send
> -             ? netdev->netdev_class->send(netdev, qid, buffers, cnt, may_steal)
> -             : EOPNOTSUPP);
> +    int error = netdev->netdev_class->send(netdev, qid, buffers, cnt,
> +                                           may_steal);
>     if (!error) {
>         COVERAGE_INC(netdev_sent);
>     }
> -- 
> 2.1.3
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev