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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
