<div dir="ltr">Thanks Ben,<div><br></div><div style>This patch is more for making my ofp/odp naming project easier. I'll post a V2 of it.</div><div><br><div class="gmail_extra">On Tue, Jun 4, 2013 at 10:17 AM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>></span> wrote:<br>
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, May 31, 2013 at 04:04:45PM -0700, Alex Wang wrote:<br>
> This patch changes the variable type of "ofport" in "struct if_cfg" and<br>
> "struct iface" from int64_t to uint16_t. This is more consistent with<br>
> the OpenFlow-1.0 port definition.<br>
><br>
> Also, before this patch, -1 is used to indicate an unknown port. This<br>
> patch uses OFPP_NONE, since "ofport" becomes uint16_t.<br>
><br>
> Signed-off-by: Alex Wang <<a href="mailto:alexw@nicira.com">alexw@nicira.com</a>><br>
<br>
</div>Thank you. I agree that this helps make the code a little cleaner. I<br>
have a couple of comments.<br>
<br>
First, what is the purpose of the casts here:<br>
<div class="im"><br>
> +static uint16_t<br>
> iface_pick_ofport(const struct ovsrec_interface *cfg)<br>
> {<br>
> - int64_t ofport = cfg->n_ofport ? *cfg->ofport : OFPP_NONE;<br>
> - return cfg->n_ofport_request ? *cfg->ofport_request : ofport;<br>
> + uint16_t ofport = cfg->n_ofport ? (uint16_t) *cfg->ofport : OFPP_NONE;<br>
> + return cfg->n_ofport_request ? (uint16_t) *cfg->ofport_request : ofport;<br></div></blockquote><div><br></div><div style><br></div><div style>You are right. There is no need for explicit cast here.</div><div style>
<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
</div>Second, iface_configure_qos() tests ->ofp_port for >= 0, which no<br>
longer makes sense. I guess it should test for ->ofp_port !=<br>
OFPP_NONE now.<br></blockquote><div><br></div><div style>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</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks,<br>
<br>
Ben.<br>
</blockquote></div><br></div></div></div>