I pushed this change with a slight modification. I changed this:
if (++ofproto->alloc_port_no == ofproto->max_ports) {
to this:
if (++ofproto->alloc_port_no >= ofproto->max_ports) {
Your code was correct, but this just seems a bit safer.
Thanks for the fix!
--Justin
On Jan 10, 2013, at 9:20 PM, Krishna Kondaka <[email protected]> wrote:
> From: Krishna Kondaka <[email protected]>
>
> alloc_ofp_port() does not allocate the port number correctly if the port
> number passed initially is already in use. The following if block
>
> if (ofp_port >= ofproto->max_ports
> || bitmap_is_set(ofproto->ofp_port_ids, ofp_port)) {
>
> is entered when either of the two conditions is true but the while block
> after this is not entered if the second condition above is true and the
> first condition is false.
>
> This results in an existing port_number to be re-assigned!
>
> Signed-off-by: Krishna Kondaka <[email protected]>
> ---
> AUTHORS | 1 +
> ofproto/ofproto.c | 30 +++++++++++-------------------
> 2 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/AUTHORS b/AUTHORS
> index 0639f7e..b34287e 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -44,6 +44,7 @@ Joe Stringer [email protected]
> Jun Nakajima [email protected]
> Justin Pettit [email protected]
> Keith Amidon [email protected]
> +Krishna Kondaka [email protected]
> Kyle Mestery [email protected]
> Leo Alterman [email protected]
> Luca Giraudo [email protected]
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 98515c2..83d4759 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1613,38 +1613,30 @@ static uint16_t
> alloc_ofp_port(struct ofproto *ofproto, const char *netdev_name)
> {
> uint16_t ofp_port;
> + uint16_t end_port_no = ofproto->alloc_port_no;
>
> ofp_port = simap_get(&ofproto->ofp_requests, netdev_name);
> ofp_port = ofp_port ? ofp_port : OFPP_NONE;
>
> if (ofp_port >= ofproto->max_ports
> || bitmap_is_set(ofproto->ofp_port_ids, ofp_port)) {
> - bool retry = ofproto->alloc_port_no ? true : false;
> -
> /* Search for a free OpenFlow port number. We try not to
> * immediately reuse them to prevent problems due to old
> * flows. */
> - while (ofp_port >= ofproto->max_ports) {
> - for (ofproto->alloc_port_no++;
> - ofproto->alloc_port_no < ofproto->max_ports;
> - ofproto->alloc_port_no++) {
> - if (!bitmap_is_set(ofproto->ofp_port_ids,
> - ofproto->alloc_port_no)) {
> - ofp_port = ofproto->alloc_port_no;
> - break;
> - }
> + for (;;) {
> + if (++ofproto->alloc_port_no == ofproto->max_ports) {
> + ofproto->alloc_port_no = 0;
> }
> - if (ofproto->alloc_port_no >= ofproto->max_ports) {
> - if (retry) {
> - ofproto->alloc_port_no = 0;
> - retry = false;
> - } else {
> - return OFPP_NONE;
> - }
> + if (!bitmap_is_set(ofproto->ofp_port_ids,
> + ofproto->alloc_port_no)) {
> + ofp_port = ofproto->alloc_port_no;
> + break;
> + }
> + if (ofproto->alloc_port_no == end_port_no) {
> + return OFPP_NONE;
> }
> }
> }
> -
> bitmap_set1(ofproto->ofp_port_ids, ofp_port);
> return ofp_port;
> }
> --
> 1.7.4.1
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
