On Tue, Jan 19, 2016 at 02:07:55PM -0800, Jarno Rajahalme wrote:
> With clarifications requested below:
>
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
>
> > On Jan 18, 2016, at 11:27 PM, Ben Pfaff <b...@ovn.org> wrote:
> >
> > Signed-off-by: Ben Pfaff <b...@ovn.org>
> > ---
> > lib/ofp-prop.c | 98 +++++++++++++++++++++++++++++++++++------
> > lib/ofp-prop.h | 57 ++++++++++++++++++++----
> > lib/ofp-util.c | 134
> > ++++++++++++---------------------------------------------
> > 3 files changed, 160 insertions(+), 129 deletions(-)
> >
> > diff --git a/lib/ofp-prop.c b/lib/ofp-prop.c
> > index 2d90e1b..83d72ab 100644
> > --- a/lib/ofp-prop.c
> > +++ b/lib/ofp-prop.c
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2014, 2015 Nicira, Inc.
> > + * Copyright (c) 2014, 2015, 2016 Nicira, Inc.
> > *
> > * Licensed under the Apache License, Version 2.0 (the "License");
> > * you may not use this file except in compliance with the License.
> > @@ -21,8 +21,21 @@
> > #include "byte-order.h"
> > #include "ofpbuf.h"
> > #include "ofp-errors.h"
> > +#include "openvswitch/vlog.h"
> > #include "util.h"
> >
> > +static uint32_t
> > +ofpprop_type_to_experimenter(uint64_t type)
> > +{
> > + return type >> 32;
> > +}
>
> There are some ambiguities here. What is here termed ‘experimenter’ is
> elsewhere also called ‘experimenter ID’, and what is below termed
> ‘exp_id’ is elsewhere also called 'experimenter type’ or ‘ex_type’.
This was basically just wrong, oops.
For the record, an "experimenter" or an "experimenter ID" are supposed
to be the same thing; an "experimenter type" or exp_type is something
like a subtype.
I changed all the terminology to fix the problem.
> > +static uint32_t
> > +ofpprop_type_to_exp_id(uint64_t type)
> > +{
> > + return type & UINT32_MAX;
> > +}
> > +
> > /* 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
> > @@ -33,7 +46,8 @@
> > * you can use ofpprop_pull() for that case. */
> > enum ofperr
> > ofpprop_pull__(struct ofpbuf *msg, struct ofpbuf *property,
> > - unsigned int alignment, uint16_t *typep)
> > + unsigned int alignment, unsigned int min_exp,
>
> It would be nice to justify the existence of ‘min_exp’ in the comment above.
I added a comment:
* This function treats property types 'min_exp' and larger as introducing
* experimenter properties. For most kinds of properties, 0xffff is the
* appropriate value for 'min_exp', because 0xffff is the only property type
* used for experimenters, but async config properties also use 0xfffe. Use
* 0x10000 (or higher) if experimenter properties are not supported.
> > -ofpprop_pull(struct ofpbuf *msg, struct ofpbuf *property, uint16_t *typep)
> > +ofpprop_pull(struct ofpbuf *msg, struct ofpbuf *property, uint64_t *typep)
> > {
> > - return ofpprop_pull__(msg, property, 8, typep);
> > + return ofpprop_pull__(msg, property, 8, 0xffff, typep);
>
> It would be nice to document the use of '0xffff’ here.
I think the new comment on ofpprop_pull__() makes it clear.
> > + } else {
> > + vlog_rate_limit(module, level, &rl,
> > + "unknown %s property type for experimenter "
> > + "0x%"PRIx32", exp_id %”PRId32,
>
> The terminology ambiguity comment applies here as well (exp ID vs. exp type).
Fixed.
> > + * - OpenFlow assigns 16-bit type values to its own standardized
> > + * properties. ofpprop uses these values directly in uint64_t.
> > + *
>
> Maybe mention that in some cases high 16-bit values denote experimenter types?
Done.
> > - error = ofpprop_pull__(payload, NULL, 1, &type);
> > + error = ofpprop_pull__(payload, NULL, 1, 0x10000, &type);
>
> It would be nice to document the significance of the value ‘0x10000’ here.
I think the new comment on ofpprop_pull__() makes it clear.
I'll push this in a minute. Thanks again.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
