[ovs-dev] [PATCH V1] bridge.c: Change variable "ofport" type in "struct if_cfg" and "struct iface"

[Alex Wang]

Thanks Ben,

This patch is more for making my ofp/odp naming project easier. I'll post a
V2 of it.

On Tue, Jun 4, 2013 at 10:17 AM, Ben Pfaff <blp at nicira.com> wrote:

> On Fri, May 31, 2013 at 04:04:45PM -0700, Alex Wang wrote:
> > This patch changes the variable type of "ofport" in "struct if_cfg" and
> > "struct iface" from int64_t to uint16_t. This is more consistent with
> > the OpenFlow-1.0 port definition.
> >
> > Also, before this patch, -1 is used to indicate an unknown port. This
> > patch uses OFPP_NONE, since "ofport" becomes uint16_t.
> >
> > Signed-off-by: Alex Wang <alexw at nicira.com>
>
> Thank you.  I agree that this helps make the code a little cleaner.  I
> have a couple of comments.
>
> First, what is the purpose of the casts here:
>
> > +static uint16_t
> >  iface_pick_ofport(const struct ovsrec_interface *cfg)
> >  {
> > -    int64_t ofport = cfg->n_ofport ? *cfg->ofport : OFPP_NONE;
> > -    return cfg->n_ofport_request ? *cfg->ofport_request : ofport;
> > +    uint16_t ofport = cfg->n_ofport ? (uint16_t) *cfg->ofport :
> OFPP_NONE;
> > +    return cfg->n_ofport_request ? (uint16_t) *cfg->ofport_request :
> ofport;
>


You are right. There is no need for explicit cast here.



> Second, iface_configure_qos() tests ->ofp_port for >= 0, which no
> longer makes sense.  I guess it should test for ->ofp_port !=
> OFPP_NONE now.
>

Thanks for finding this out. This is what I'm most afraid of, in doing such
change. Just wish I can catch them all. ;D


> Thanks,
>
> Ben.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130604/60b87f9b/attachment-0003.html>