[ovs-dev] [PATCH 18/41] ofp-prop: Add helpers for u8, be64/u64, flag, and UUID properties.

[Ben Pfaff]

Thanks, applied to master.

On Tue, Jan 19, 2016 at 02:36:51PM -0800, Jarno Rajahalme wrote:
> Small nits below,
> 
> Acked-by: Jarno Rajahalme <jarno at ovn.org>
> 
> > On Jan 18, 2016, at 11:27 PM, Ben Pfaff <blp at ovn.org> wrote:
> > 
> > /* Pulls a property, beginning with struct ofp_prop_header, from the beginning
> >  * of 'msg'.  Stores the type of the property in '*typep' and, if 'property' is
> > - * nonnull, the entire property, including the header, in '*property'.  Returns
> > - * 0 if successful, otherwise an error code.
> > + * nonnull, the entire property, including the header, in '*property'.  Points
> > + * 'property->msg' to just past the property header (which could be
> > + * ofp_prop_header or ofp_prop_experimenter) in 'property'.  Returns 0 if
> > + * successful, otherwise an error code.
> 
> Maybe should mention the setting of ‘property->header’ as well,
> esp. since it is used by the new ofpbuf_headersize() function?

That's a good idea.

I changed the added sentence to:

 * Points 'property->header' to the property header (which could be
 * ofp_prop_header or ofp_prop_experimenter) and 'property->msg' to just
 * past it.

> > +/* Attempts to parse 'property' as a property containing a 64-bit value.  If
> > + * successful, stores the value into '*value' and returns 0; otherwise returns
> > + * an OpenFlow error. */
> > +enum ofperr
> > +ofpprop_parse_be64(const struct ofpbuf *property, ovs_be64 *value)
> > +{
> > +    size_t be64_offset = ROUND_UP(ofpbuf_headersize(property), 8);
> > +    if (property->size != be64_offset + 8) {
> 
> Maybe use 'sizeof *p' here as well for consistency?

Thanks; there was a nastier bug here too: below, the '8' should have
been be64_offset.  I fixed that too.

> > +        return OFPERR_OFPBPC_BAD_LEN;
> > +    }
> > +
> > +    ovs_be64 *p = ALIGNED_CAST(ovs_be64 *, (char *) property->data + 8);
> > +    *value = *p;
> > +    return 0;
> > +}

...

> > +/* Attempts to parse 'property' as a property containing a 64-bit value.  If
> > + * successful, stores the value into '*value' and returns 0; otherwise returns
> > + * an OpenFlow error. */
> > +enum ofperr
> > +ofpprop_parse_u64(const struct ofpbuf *property, uint64_t *value)
> > +{
> > +    size_t be64_offset = ROUND_UP(ofpbuf_headersize(property), 8);
> > +    if (property->size != be64_offset + 8) {
> 
> And here?

Ditto, thanks.