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

Justin Pettit jpettit at nicira.com
Mon Dec 6 08:03:52 UTC 2010


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?

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

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

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

Thanks for cleaning this code all up.

--Justin






More information about the dev mailing list