[ovs-dev] [pre-async 2/8] openflow: Merge ofp_stats_request and ofp_stats_reply.

Justin Pettit jpettit at nicira.com
Thu May 26 07:49:13 UTC 2011


On May 24, 2011, at 4:25 PM, Ben Pfaff wrote:

> -/* Creates an ofp_stats_request with the given 'type' and 'body_len' bytes of
> - * space allocated for the 'body' member.  Returns the first byte of the 'body'
> +/* Creates an ofp_stats_msg with the given 'type' and 'body_len' bytes of space
> + * allocated for the 'body' member.  Returns the first byte of the 'body'
>  * member. */

Peeking ahead at the next patch, I see you remove the "body" member.  In that commit you don't update this comment to remove the reference to "body", but you do remove the other references in this patch.

> -    struct ofp_stats_request *osr;
> ...
> +    struct ofp_stats_msg *osr;

This is pretty minor, but our general convention would be to call this "osm" instead of "osr".  I noticed that updating the name is done is some locations of this patch, but not others.

> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 5f9c830..52e89d0 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -262,8 +262,8 @@ open_vconn(const char *name, struct vconn **vconnp)
> static void *
> alloc_stats_request(size_t body_len, uint16_t type, struct ofpbuf **bufferp)
> {
> -    struct ofp_stats_request *rq;
> -    rq = make_openflow((offsetof(struct ofp_stats_request, body)
> +    struct ofp_stats_msg *rq;
> +    rq = make_openflow((offsetof(struct ofp_stats_msg, body)
>                         + body_len), OFPT_STATS_REQUEST, bufferp);

Elsewhere in the commit, you get rid of a similar use of offsetof(), so this conversion to remove "body" is spread across two patches.  Logically, it seems like all the removals of "body" should happen in the next commit.  Obviously, it doesn't really matter for correctness, though.  (Haven't you missed my petty reviews?)

--Justin





More information about the dev mailing list