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 <b...@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 <al...@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.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
