[ovs-dev] [PATCH] alloc_ofp_port does not allocate the port number correctly

[Krishna Kondaka]

Thanks Justin!

Krishna

----- Original Message -----
From: "Justin Pettit" <jpettit at nicira.com>
To: "Krishna Kondaka" <kkondaka at vmware.com>
Cc: dev at openvswitch.org
Sent: Friday, January 11, 2013 3:10:41 PM
Subject: Re: [ovs-dev] [PATCH] alloc_ofp_port does not allocate the port number correctly

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 <kkondaka at vmware.com> wrote:

> From: Krishna Kondaka <kkondaka at vmware.com>
> 
> 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 <kkondaka at vmware.com>
> ---
> 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            joe at wand.net.nz
> Jun Nakajima            jun.nakajima at intel.com
> Justin Pettit           jpettit at nicira.com
> Keith Amidon            keith at nicira.com
> +Krishna Kondaka         kkondaka at vmware.com
> Kyle Mestery            kmestery at cisco.com
> Leo Alterman            lalterman at nicira.com
> Luca Giraudo            lgiraudo at nicira.com
> 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
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev