[ovs-dev] [PATCH v4 06/14] ofp-prop: Add support for putting and parsing nested properties.

[Ben Pfaff]

On Fri, Feb 19, 2016 at 02:47:59PM -0800, Jarno Rajahalme wrote:
> With one comment below:
> 
> Acked-by: Jarno Rajahalme <jarno at ovn.org>

Thanks for the review.

> > On Feb 19, 2016, at 12:34 AM, Ben Pfaff <blp at ovn.org> wrote:
> > 
> > It hadn't occurred to me before that any special support was actually
> > necessary or useful for nested properties, but the functions introduced in
> > this commit are nice wrappers to deal with the extra 4-byte padding that
> > ensures that the nested properties begin on 8-byte boundaries just like
> > the outer properties.
> > 
> > Signed-off-by: Ben Pfaff <blp at ovn.org>

...

> > +enum ofperr
> > +ofpprop_parse_nested(const struct ofpbuf *property, struct ofpbuf *nested)
> > +{
> > +    size_t nested_offset = ROUND_UP(ofpbuf_headersize(property), 8);
> > +    if (property->size < nested_offset) {
> > +        return OFPERR_OFPBPC_BAD_LEN;
> > +    }
> > +
> > +    ofpbuf_use_const(nested, property->data, property->size);
> > +    ofpbuf_pull(nested, nested_offset);
> > +    return 0;
> > +}

...

> > +/* Appends a header for a property of type 'type' to 'msg', followed by padding
> > + * suitable for putting nested properties into the property; that is, padding
> > + * to an 8-byte alignment.
> > + *
> > + * This otherwise works like ofpprop_start().
> > + *
> > + * There's no need for ofpprop_end_nested(), because ofpprop_end() works fine
> > + * for this case. */
> > +size_t
> > +ofpprop_start_nested(struct ofpbuf *msg, uint64_t type)
> > +{
> > +    size_t start_ofs = ofpprop_start(msg, type);
> > +    ofpbuf_put_zeros(msg, 4);
> 
> Somehow I find the ‘4’ here asymmetric to the
> ROUND_UP(ofpbuf_headersize(…)..) in ofpprop_parse_nested(). Maybe it
> would be better to simply pull ‘8’ there (with the comment that both
> the outer property and the padding are bypassed?

Hmm, 8 would be wrong sometimes there, because experimenter property
headers are 12 bytes.

Do you like this better:
    ofpbuf_padto(msg, ROUND_UP(msg->size, 8));