[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openvswitch-dev
Subject:    [ovs-dev] [PATCH 1/3] ovs-ofctl: Add "queue-stats" command to print queue stats.
From:       jpettit () nicira ! com (Justin Pettit)
Date:       2010-09-30 5:26:53
Message-ID: 1825AF14-BA01-40AE-959D-BAAB50F88C4F () nicira ! com
[Download RAW message or body]

On Sep 16, 2010, at 3:42 PM, Ben Pfaff wrote:

> +static void
> +ofp_queue_stats_request(struct ds *string, const void *body_,
> +                       size_t len OVS_UNUSED, int verbosity OVS_UNUSED)
> +{
> ...
> +    ds_put_cstr(string, "port_no=");
> +    ofp_print_port_name(string, ntohs(qsr->port_no));
> +    ds_put_cstr(string, " queue_id=");
> +    ofp_print_queue_name(string, ntohl(qsr->queue_id));
> +}
> +
> +static void
> +ofp_queue_stats_reply(struct ds *string, const void *body, size_t len,
> +                     int verbosity)
> +...
> +    for (; n--; qs++) {
> +        ds_put_cstr(string, "  port ");
> +        ofp_print_port_name(string, ntohs(qs->port_no));
> +        ds_put_cstr(string, " queue ");
> +        ofp_print_queue_name(string, ntohl(qs->queue_id));
> +        ds_put_cstr(string, ": ");

In ofp_queue_stats_request, it's printed as "port_no", but in ofp_queue_stats_reply \
it is just "port".  I think it would be better to be consistent and preferably match \
the man page.  The same is true for "queue_id" and "queue".

 (I'll also point out that the other patch series I reviewed today used "port-name" \
and "queue-id" in ovs-controller).

> +.IP "\fBqueue\-stats \fIswitch \fR[\fIport \fR[\fIqueue\fR]]"
> +Prints to the console statistics for the specified \fIqueue\fR on
> +\fIport\fR within \fIswitch\fR.  Either of \fIport\fR or \fIqueue\fR
> +or both may be omitted (or equivalently specified as \fBALL\fR).  If

This seems to imply that "port" can be not specified and "queue" can be, which I \
don't think is true.  

Also, I'll put in a vote for using "port-name" and "queue-id" throughout, since it \
seems a bit clearly the preferred type of value that should be given.

Thanks for doing this.  It will make debugging QoS much easier.

--Justin


[prev in list] [next in list] [prev in thread] [next in thread]