[ovs-dev] [pre-async 6/8] openflow: Make stats replies more like other OpenFlow messages.

Ben Pfaff blp at nicira.com
Tue May 31 23:46:08 UTC 2011


On Tue, May 31, 2011 at 12:33:43AM -0700, Justin Pettit wrote:
> 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.)

I'll just delete the osm->flags assignment, thanks.

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

Thanks, fixed.

> > +/* 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".)

Thanks, I fixed this one too.

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

Yeah, I'll do that.  Thanks for pointing it out.

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

That's true of any ofpbuf_* function that expands the data area.  It's
nothing specific to ofpbuf_padto().

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

Good point, I removed 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().

Can't hurt.  Done.



More information about the dev mailing list