[ovs-dev] [queue-stats v2 2/2] ofproto: Report nonexistent ports and queues as errors in queue stats.

[Pravin Shelar]

Looks good.

On Wed, Jun 27, 2012 at 10:05 AM, Ben Pfaff <blp at nicira.com> wrote:
> Until now, Open vSwitch has ignored missing ports and queues in most cases
> in queue stats requests, simply returning an empty set of statistics.
> It seems that it is better to report an error, so this commit does this.
>
> Reported-by: Prabina Pattnaik <Prabina.Pattnaik at nechclst.in>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> v1 was here: http://openvswitch.org/pipermail/dev/2012-June/017780.html
> v2: no change
>
>  AUTHORS           |    1 +
>  NEWS              |    4 ++++
>  ofproto/ofproto.c |   28 ++++++++++++++++++----------
>  tests/ofproto.at  |    8 ++++++++
>  4 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index bdfcb33..dc98ebe 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -140,6 +140,7 @@ Paul Ingram             paul at nicira.com
>  Paulo Cravero           pcravero at as2594.net
>  Peter Balland           peter at nicira.com
>  Peter Phaal             peter.phaal at inmon.com
> +Prabina Pattnaik        Prabina.Pattnaik at nechclst.in
>  Ralf Heiringhoff        ralf at frosty-geek.net
>  Ram Jothikumar          rjothikumar at nicira.com
>  Ramana Reddy            gtvrreddy at gmail.com
> diff --git a/NEWS b/NEWS
> index c2ec953..f0b2490 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -11,6 +11,10 @@ post-v1.7.0
>         the multicast bit in the destination address could be individually
>        masked.)
>       - New field OXM_OF_METADATA, to align with OpenFlow 1.1.
> +      - The OFPST_QUEUE request now reports an error if a specified port or
> +        queue does not exist, or for requests for a specific queue on all
> +        ports, if the specified queue does not exist on any port.  (Previous
> +        versions generally reported an empty set of results.)
>     - Additional protocols are not mirrored and dropped when forward-bpdu is
>       false.  For a full list, see the ovs-vswitchd.conf.db man page.
>     - Open vSwitch now sends RARP packets in situations where it previously
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 1a2f712..ce4da9d 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2684,7 +2684,7 @@ handle_queue_stats_dump_cb(uint32_t queue_id,
>     put_queue_stats(cbdata, queue_id, stats);
>  }
>
> -static void
> +static enum ofperr
>  handle_queue_stats_for_port(struct ofport *port, uint32_t queue_id,
>                             struct queue_stats_cbdata *cbdata)
>  {
> @@ -2697,8 +2697,11 @@ handle_queue_stats_for_port(struct ofport *port, uint32_t queue_id,
>
>         if (!netdev_get_queue_stats(port->netdev, queue_id, &stats)) {
>             put_queue_stats(cbdata, queue_id, &stats);
> +        } else {
> +            return OFPERR_OFPQOFC_BAD_QUEUE;
>         }
>     }
> +    return 0;
>  }
>
>  static enum ofperr
> @@ -2707,9 +2710,10 @@ handle_queue_stats_request(struct ofconn *ofconn,
>  {
>     struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
>     struct queue_stats_cbdata cbdata;
> -    struct ofport *port;
>     unsigned int port_no;
> +    struct ofport *port;
>     uint32_t queue_id;
> +    enum ofperr error;
>
>     COVERAGE_INC(ofproto_queue_req);
>
> @@ -2718,21 +2722,25 @@ handle_queue_stats_request(struct ofconn *ofconn,
>     port_no = ntohs(qsr->port_no);
>     queue_id = ntohl(qsr->queue_id);
>     if (port_no == OFPP_ALL) {
> +        error = OFPERR_OFPQOFC_BAD_QUEUE;
>         HMAP_FOR_EACH (port, hmap_node, &ofproto->ports) {
> -            handle_queue_stats_for_port(port, queue_id, &cbdata);
> +            if (!handle_queue_stats_for_port(port, queue_id, &cbdata)) {
> +                error = 0;
> +            }
>         }
> -    } else if (port_no < OFPP_MAX) {
> +    } else {
>         port = ofproto_get_port(ofproto, port_no);
> -        if (port) {
> -            handle_queue_stats_for_port(port, queue_id, &cbdata);
> -        }
> +        error = (port
> +                 ? handle_queue_stats_for_port(port, queue_id, &cbdata)
> +                 : OFPERR_OFPQOFC_BAD_PORT);
> +    }
> +    if (!error) {
> +        ofconn_send_replies(ofconn, &cbdata.replies);
>     } else {
>         ofpbuf_list_delete(&cbdata.replies);
> -        return OFPERR_OFPQOFC_BAD_PORT;
>     }
> -    ofconn_send_replies(ofconn, &cbdata.replies);
>
> -    return 0;
> +    return error;
>  }
>
>  static bool
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 28adf74..d703fa8 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -59,6 +59,14 @@ AT_CHECK([ovs-ofctl -vwarn queue-stats br0], [0], [stdout])
>  AT_CHECK([STRIP_XIDS stdout], [0], [dnl
>  OFPST_QUEUE reply: 0 queues
>  ])
> +AT_CHECK([ovs-ofctl -vwarn queue-stats br0 ALL 5], [0],
> +  [OFPT_ERROR (xid=0x1): OFPQOFC_BAD_QUEUE
> +OFPST_QUEUE request (xid=0x1):port=ALL queue=5
> +])
> +AT_CHECK([ovs-ofctl -vwarn queue-stats br0 10], [0],
> +  [OFPT_ERROR (xid=0x1): OFPQOFC_BAD_PORT
> +OFPST_QUEUE request (xid=0x1):port=10 queue=ALL
> +])
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev