[ovs-dev] [PATCH 1/3] ovs-ofctl: Add "queue-stats" command to print queue stats.

[Justin Pettit]

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