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

[Pravin Shelar]

On Mon, Jun 11, 2012 at 11:26 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>
> ---
>  AUTHORS           |    1 +
>  NEWS              |    7 ++++++-
>  ofproto/ofproto.c |   28 ++++++++++++++++++----------
>  tests/ofproto.at  |    8 ++++++++
>  4 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 10a01ab..801d31a 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -137,6 +137,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
>  Ram Jothikumar          rjothikumar at nicira.com
>  Ramana Reddy            gtvrreddy at gmail.com
>  Rob Sherwood            rob.sherwood at bigswitch.com
> diff --git a/NEWS b/NEWS
> index 8cd38dd..b96ced8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,7 +2,12 @@ post-v1.7.0
>  ------------------------
>     - ovs-ofctl:
>         - "mod-port" command can now control all OpenFlow config flags.
> -    - Added support for arbitrary ethernet masks
> +    - OpenFlow:
> +        - Added support for arbitrary ethernet masks
> +        - 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 dee2cbc..8f7d05c 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2644,7 +2644,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)
>  {
> @@ -2657,8 +2657,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
> @@ -2667,9 +2670,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);
>
> @@ -2678,21 +2682,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;
> +            }
in case of multiple ports this error code could reset.

>         }
> -    } 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 dbe31c4..a193490 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
> +])

expected error code from ofctl shld be 1.
> +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

I am seeing this test failure on my machine.

>
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev