[ovs-dev] [PATCH] ofp-util: Make ofputil_port_from_ofp11() return OFPP_NONE on error.

[Ben Pfaff]

Thanks, I'll push this soon.

I think it's too strong to call this a "policy".  It's more like my
personal preference (since it's not mentioned in CodingStyle and I
don't think we've had any discussion of it here).  I edited the commit
message to better reflect that.

On Mon, May 20, 2013 at 07:13:54PM +0000, Rajahalme, Jarno (NSN - FI/Espoo) wrote:
> This good, and thanks for clarifying the policy on function output parameters.
> 
>   Jarno
> 
> On May 20, 2013, at 21:31 , ext Ben Pfaff wrote:
> 
> > This makes life easier for a few callers, and it agrees with a general
> > principle that a function should fill in its output parameters whether it
> > succeeds or not.
> > 
> > CC: Jarno Rajahalme <jarno.rajahalme at nsn.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > lib/meta-flow.c |   12 +++---------
> > lib/ofp-util.c  |    3 ++-
> > 2 files changed, 5 insertions(+), 10 deletions(-)
> > 
> > diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> > index c59d82a..54bc4c2 100644
> > --- a/lib/meta-flow.c
> > +++ b/lib/meta-flow.c
> > @@ -1203,9 +1203,7 @@ mf_set_value(const struct mf_field *mf,
> > 
> >     case MFF_IN_PORT_OXM: {
> >         uint16_t port;
> > -        if (ofputil_port_from_ofp11(value->be32, &port)) {
> > -            port = OFPP_NONE;
> > -        }
> > +        ofputil_port_from_ofp11(value->be32, &port);
> >         match_set_in_port(match, port);
> >         break;
> >     }
> > @@ -1395,9 +1393,7 @@ mf_set_flow_value(const struct mf_field *mf,
> > 
> >     case MFF_IN_PORT_OXM: {
> >         uint16_t port;
> > -        if (ofputil_port_from_ofp11(value->be32, &port)) {
> > -            port = OFPP_NONE;
> > -        }
> > +        ofputil_port_from_ofp11(value->be32, &port);
> >         flow->in_port = port;
> >         break;
> >     }
> > @@ -2481,9 +2477,7 @@ mf_format(const struct mf_field *mf,
> >     case MFS_OFP_PORT_OXM:
> >         if (!mask) {
> >             uint16_t port;
> > -            if (ofputil_port_from_ofp11(value->be32, &port)) {
> > -                port = OFPP_NONE;
> > -            }
> > +            ofputil_port_from_ofp11(value->be32, &port);
> >             ofputil_format_port(port, s);
> >             break;
> >         }
> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > index 2ca0077..b4ff09b 100644
> > --- a/lib/ofp-util.c
> > +++ b/lib/ofp-util.c
> > @@ -4029,7 +4029,7 @@ ofputil_frag_handling_from_string(const char *s, enum ofp_config_flags *flags)
> > /* Converts the OpenFlow 1.1+ port number 'ofp11_port' into an OpenFlow 1.0
> >  * port number and stores the latter in '*ofp10_port', for the purpose of
> >  * decoding OpenFlow 1.1+ protocol messages.  Returns 0 if successful,
> > - * otherwise an OFPERR_* number.
> > + * otherwise an OFPERR_* number.  On error, stores OFPP_NONE in '*ofp10_port'.
> >  *
> >  * See the definition of OFP11_MAX for an explanation of the mapping. */
> > enum ofperr
> > @@ -4044,6 +4044,7 @@ ofputil_port_from_ofp11(ovs_be32 ofp11_port, uint16_t *ofp10_port)
> >         *ofp10_port = ofp11_port_h - OFPP11_OFFSET;
> >         return 0;
> >     } else {
> > +        *ofp10_port = OFPP_NONE;
> >         VLOG_WARN_RL(&bad_ofmsg_rl, "port %"PRIu32" is outside the supported "
> >                      "range 0 through %d or 0x%"PRIx32" through 0x%"PRIx32,
> >                      ofp11_port_h, OFPP_MAX - 1,
> > -- 
> > 1.7.2.5
> > 
>