Thanks, I'll apply this in a minute.
On Fri, Aug 08, 2014 at 12:31:51PM -0700, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> On Aug 7, 2014, at 4:13 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > Parsing of actions was wrong because OF1.3 says that non-experimenter
> > actions are 4 bytes and don't include padding. This fixes the problem.
> >
> > Parsing of instructions seems wrong too because OF1.3 says that
> > non-experimenter instructions are 4 bytes (though it is not explicit about
> > padding as it is for actions). This fixes the problem there too.
> >
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > lib/ofp-util.c | 47 +++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 39 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > index 1e641bd..1ba3970 100644
> > --- a/lib/ofp-util.c
> > +++ b/lib/ofp-util.c
> > @@ -61,12 +61,17 @@ struct ofp_prop_header {
> > /* 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. */
> > + * 0 if successful, otherwise an error code.
> > + *
> > + * This function pulls the property's stated size padded out to a multiple
> > of
> > + * 'alignment' bytes. The common case in OpenFlow is an 'alignment' of 8,
> > so
> > + * you can use ofputil_pull_property() for that case. */
> > static enum ofperr
> > -ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf *property,
> > - uint16_t *typep)
> > +ofputil_pull_property__(struct ofpbuf *msg, struct ofpbuf *property,
> > + unsigned int alignment, uint16_t *typep)
> > {
> > struct ofp_prop_header *oph;
> > + unsigned int padded_len;
> > unsigned int len;
> >
> > if (ofpbuf_size(msg) < sizeof *oph) {
> > @@ -75,7 +80,8 @@ ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf
> > *property,
> >
> > oph = ofpbuf_data(msg);
> > len = ntohs(oph->len);
> > - if (len < sizeof *oph || ROUND_UP(len, 8) > ofpbuf_size(msg)) {
> > + padded_len = ROUND_UP(len, alignment);
> > + if (len < sizeof *oph || padded_len > ofpbuf_size(msg)) {
> > return OFPERR_OFPBPC_BAD_LEN;
> > }
> >
> > @@ -83,10 +89,24 @@ ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf
> > *property,
> > if (property) {
> > ofpbuf_use_const(property, ofpbuf_data(msg), len);
> > }
> > - ofpbuf_pull(msg, ROUND_UP(len, 8));
> > + ofpbuf_pull(msg, padded_len);
> > return 0;
> > }
> >
> > +/* 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.
> > + *
> > + * This function pulls the property's stated size padded out to a multiple
> > of
> > + * 8 bytes, which is the common case for OpenFlow properties. */
> > +static enum ofperr
> > +ofputil_pull_property(struct ofpbuf *msg, struct ofpbuf *property,
> > + uint16_t *typep)
> > +{
> > + return ofputil_pull_property__(msg, property, 8, typep);
> > +}
> > +
> > static void PRINTF_FORMAT(2, 3)
> > log_property(bool loose, const char *message, ...)
> > {
> > @@ -4443,10 +4463,12 @@ ofputil_encode_port_mod(const struct
> > ofputil_port_mod *pm,
> >
> > return b;
> > }
> > +
> > +/* Table features. */
> >
> > static enum ofperr
> > pull_table_feature_property(struct ofpbuf *msg, struct ofpbuf *payload,
> > - uint16_t *typep)
> > + uint16_t *typep)
> > {
> > enum ofperr error;
> >
> > @@ -4467,7 +4489,7 @@ parse_action_bitmap(struct ofpbuf *payload, enum
> > ofp_version ofp_version,
> > uint16_t type;
> > enum ofperr error;
> >
> > - error = pull_table_feature_property(payload, NULL, &type);
> > + error = ofputil_pull_property__(payload, NULL, 1, &type);
> > if (error) {
> > return error;
> > }
> > @@ -4489,7 +4511,16 @@ parse_instruction_ids(struct ofpbuf *payload, bool
> > loose, uint32_t *insts)
> > enum ofperr error;
> > uint16_t ofpit;
> >
> > - error = pull_table_feature_property(payload, NULL, &ofpit);
> > + /* OF1.3 and OF1.4 aren't clear about padding in the instruction
> > IDs.
> > + * It seems clear that they aren't padded to 8 bytes, though,
> > because
> > + * both standards say that "non-experimenter instructions are 4
> > bytes"
> > + * and do not mention any padding before the first instruction ID.
> > + * (There wouldn't be any point in padding to 8 bytes if the IDs
> > were
> > + * aligned on an odd 4-byte boundary.)
> > + *
> > + * Anyway, we just assume they're all glommed together on byte
> > + * boundaries. */
> > + error = ofputil_pull_property__(payload, NULL, 1, &ofpit);
> > if (error) {
> > return error;
> > }
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
