[ovs-dev] [tests+nxm-ofctl 34/42] Refactor and centralize basic OpenFlow message decoding and validation.

Ben Pfaff blp at nicira.com
Mon Dec 6 18:19:07 UTC 2010


On Mon, Dec 06, 2010 at 12:03:52AM -0800, Justin Pettit wrote:
> On Nov 23, 2010, at 2:44 PM, Ben Pfaff wrote:
> 
> > +ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh,
> > +                           int verbosity)
> > {
> > +    const struct ofp_port_stats *ps = ofputil_stats_body(oh);
> > +    size_t n = ntohs(oh->length) / sizeof *ps;
> ...
> > static void
> > +ofp_print_ofpst_table_reply(struct ds *string, const struct ofp_header *oh,
> > +                            int verbosity)
> > {
> > +    const struct ofp_table_stats *ts = ofputil_stats_body(oh);
> > +    size_t n = ntohs(oh->length) / sizeof *ts;
> ...
> > +ofp_print_ofpst_queue_reply(struct ds *string, const struct ofp_header *oh,
> > +                            int verbosity)
> > {
> > +    const struct ofp_queue_stats *qs = ofputil_stats_body(oh);
> > +    size_t n = ntohs(oh->length) / sizeof *qs;
> 
> Are these three "n" calculations correct?  Don't you need to subtract
> out the size of the stats reply header?

Oops, you're right.

To fix this, I folded in a later patch that I had that added a function
ofputil_stats_body_len() and used that here.

> > +static int
> > +check_nxstats_msg(const struct ofp_header *oh)
> > +{
> > ...
> > +    if (ntohs(osr->header.length) < sizeof(struct nicira_stats_msg)) {
> > +        VLOG_WARN_RL(&bad_ofmsg_rl, "truncated Nicira stats request");
> 
> This function is called for both requests and replies, so it may be
> better to replace "request" with "message".

Good point, done.

> > +/* Decodes the message type represented by 'oh'.  Returns 0 if successful or
> > + * an OpenFlow error code constructed with ofp_mkerr() on failure.  Either
> > + * way, stores in '*typep' a type structure that can be inspected with the
> > + * ofputil_msg_type_*() functions.
> > + *
> > + * Success indicates that 'oh' is at least as long as the minimum-length
> > + * message of its type. */
> > +int
> > +ofputil_decode_msg_type(const struct ofp_header *oh,
> > +                        const struct ofputil_msg_type **typep)
> 
> Maybe it goes without saying, but callers to this need to make sure
> that "oh->length" matches the length of received packet.

OK, I added this to the comment.

> > +/* Returns a string representing the message type of 'type'.  The string is the
> > + * enumeration constant for the type, followed by "request" or "reply" for
> > + * statistics only, e.g. "OFPT_HELLO" or "OFPST_AGGREGATE reply". */
> 
> I think it would be clearer if that second sentence were broken into
> two.  The first describes the common case, and the second describing
> stats.

OK, done.

> Thanks for cleaning this code all up.

Thanks for the review.




More information about the dev mailing list