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

Ben Pfaff blp at nicira.com
Thu May 26 16:29:16 UTC 2011


On Thu, May 26, 2011 at 12:49:13AM -0700, Justin Pettit wrote:
> 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.

Oops.  I'll fix up the comment in the next patch.

Originally patches 2 and 3 were one patch, but I split them up because
I thought it would be clearer.

> > -    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.

My reasoning here was: if an ofp_stats_msg is specifically a request
or a reply, then name it "osr" to indicate that; if it may be either
one, name it "osm".

This convention is probably too subtle and non-obvious to be
worthwhile.  So I went through and changed the requests and replies to
be named "request" or "reply" or, where there was already a variable
with that name, just "osm".

> > 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?)

I agree that's better.  I made that change.



More information about the dev mailing list