On May 24, 2011, at 4:25 PM, Ben Pfaff wrote:
> +static void
> +put_stats__(ovs_be32 xid, uint8_t ofp_type,
> + ovs_be16 ofpst_type, ovs_be32 nxst_subtype,
> + struct ofpbuf *msg)
> +{
> + if (ofpst_type == htons(OFPST_VENDOR)) {
> + struct nicira_stats_msg *nsm;
> +
> + nsm = put_openflow_xid(sizeof *nsm, ofp_type, xid, msg);
> + nsm->vsm.osm.type = ofpst_type;
Did you want to set "vsm.osm.flags" to "htons(0)" like you did below for
straight "osm"? (I don't think either is strictly necessary since
put_openflow_xid() should clear the contents.)
> + nsm->vsm.vendor = htonl(NX_VENDOR_ID);
> + nsm->subtype = nxst_subtype;
> + } else {
> + struct ofp_stats_msg *osm;
> +
> + osm = put_openflow_xid(sizeof *osm, ofp_type, xid, msg);
> + osm->type = ofpst_type;
> + osm->flags = htons(0);
> + }
> +}
...
> /* 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. */
> void *
> -ofputil_make_stats_request(size_t body_len, uint16_t type,
> - struct ofpbuf **bufferp)
> +ofputil_make_stats_request(size_t openflow_len, uint16_t ofpst_type,
> + uint32_t nxst_subtype, struct ofpbuf **bufferp)
> {
The comment describing this function should be updated.
> +/* Creates an stats reply message with the given 'type' and 'body_len' bytes
> of
> + * space allocated for the 'body' member. Returns the first byte of the
> 'body'
> + * member. */
> void *
> -ofputil_make_nxstats_request(size_t openflow_len, uint32_t subtype,
> - struct ofpbuf **bufferp)
> +ofputil_make_stats_reply(size_t openflow_len,
> + const struct ofp_stats_msg *request,
> + struct ofpbuf **bufferp)
> {
This comment should be similarly updated. (Also, the comment has "an stats".)
> +void
> +ofputil_start_stats_reply(const struct ofp_stats_msg *request,
> + struct list *replies)
> +{
> + struct ofpbuf *msg;
> +
> + msg = ofpbuf_new(1024);
> + put_stats_reply__(request, msg);
> +
> + list_init(replies);
> + list_push_back(replies, &msg->list_node);
> +}
> +
> +struct ofpbuf *
> +ofputil_reserve_stats_reply(size_t len, struct list *replies)
> +{
> + struct ofpbuf *msg = ofpbuf_from_list(list_back(replies));
> + struct ofp_stats_msg *osm = msg->data;
> +
> + if (msg->size + len <= UINT16_MAX) {
> + ofpbuf_prealloc_tailroom(msg, len);
> + } else {
> + osm->flags |= htons(OFPSF_REPLY_MORE);
> +
> + msg = ofpbuf_new(MAX(1024, sizeof(struct nicira_stats_msg) + len));
> + put_stats_reply__(osm, msg);
> + list_push_back(replies, &msg->list_node);
> + }
> + return msg;
> +}
> +
> +void *
> +ofputil_append_stats_reply(size_t len, struct list *replies)
> +{
> + return ofpbuf_put_uninit(ofputil_reserve_stats_reply(len, replies), len);
> }
It may be good to document this set of functions (ofputil_*_stats_reply()) as
the arguments and behavior are not immediately obvious.
> diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c
> index 8166d6b..3887481 100644
> --- a/lib/ofpbuf.c
> +++ b/lib/ofpbuf.c
>
> +/* If 'b' is shorter than 'length' bytes, pads its tail out with zeros to
> that
> + * length. */
> +void
> +ofpbuf_padto(struct ofpbuf *b, size_t length)
> +{
> + if (b->size < length) {
> + ofpbuf_put_zeros(b, length - b->size);
> + }
> +}
If "length" is greater than "b.allocated", couldn't this lead to a situation
where obpbuf_put_zeros() reallocates "b", and then the pointer will no longer
be valid? (I realize the current callers won't be victim to this, but it seems
dangerous from a general function.)
> static int
> handle_desc_stats_request(struct ofconn *ofconn,
> - const struct ofp_header *request)
> + const struct ofp_stats_msg *request)
> {
> struct ofproto *p = ofconn_get_ofproto(ofconn);
> struct ofp_desc_stats *ods;
> struct ofpbuf *msg;
>
> - msg = start_ofp_stats_reply(request, sizeof *ods);
> - ods = append_ofp_stats_reply(sizeof *ods, ofconn, &msg);
> + ods = ofputil_make_stats_reply(sizeof *ods, request, &msg);
> memset(ods, 0, sizeof *ods);
This memset() is probably not strictly necessary, since the ofpbuf_padto() in
ofputil_make_stats_reply() should clear it.
> @@ -2116,8 +2038,8 @@ handle_nxst_aggregate(struct ofconn *ofconn,
>
> /* Reply. */
> COVERAGE_INC(ofproto_flows_req);
> - buf = start_nxstats_reply(&request->nsm, sizeof *reply);
> - reply = ofpbuf_put_uninit(buf, sizeof *reply);
> + reply = ofputil_make_stats_reply(sizeof *reply, &request->nsm.vsm.osm,
> + &buf);
Do you think it's worth renaming "buf" to "msg", since this seems to be the
convention of callers to ofputil_make_stats_reply().
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
