Re: [ovs-dev] [PATCH] netdev-dpdk / arbitrary port naming for vhost ports

[Ben Pfaff]

On Fri, Jun 20, 2014 at 04:23:48PM +0100, maryam.tahhan wrote:
> From: "maryam.tahhan" <maryam.tah...@intel.com>
> 
> This patch enables arbitrary port naming for vhost patches, so
> they no longer need to be called dpdkvhost<n>.
> 
> Signed-off-by: maryam.tahhan <maryam.tah...@intel.com>

This is not a substantive review, but I do have a few comments.

> - **Note: tested with Qemu versions 1.4.2 and 1.6.2**
> + **Note 1: tested with Qemu versions 1.4.2 and 1.6.2**

Instead of adding the following note, perhaps your example could just
use a name other than dpdkvhost<n>:
> +   Note 2: ports don't have to be called dpdkvhost<n>.

We put a space in "if(" and before the "{" (see CodingStyle):
> +    if(!list_is_empty(&dpdk_list)){
> +        LIST_FOR_EACH (tmp_netdev, list_node, &dpdk_list) {

Please put a space after "if" and on both sides of "==" not just one:
> +            if(tmp_netdev->type== VHOST)
> +                port_no++;
> +        }
> +    }
> +
>      /* TODO: need to discover device node at run time. */
>      netdev->socket_id = SOCKET0;
>      netdev->port_id = port_no;
> @@ -505,7 +506,7 @@ netdev_dpdk_vhost_construct(struct netdev *netdev_)
>      }
>  
>      memset(&eth_addr, 0x0, sizeof(eth_addr));

OK, this code was just wrong.  Nested "if"s without indentation?  Also
we always write {} around an if's statements.  I realize that's existing
code but please fix it up as you update here.
> -    if(netdev->port_id)
> +
>      if(netdev->port_id < 0xff)
>          eth_addr.addr_bytes[5] = netdev->port_id;
>      else
> @@ -519,12 +520,13 @@ netdev_dpdk_vhost_construct(struct netdev *netdev_)
>      netdev->buf_size =
>              mbp_priv->mbuf_data_room_size - RTE_PKTMBUF_HEADROOM;
>  

Please don't put an explicit \n at the end of the log line.  Also, why
the doubled space here and in the other log message?
> +    VLOG_INFO("%s  is associated with VHOST port #%d\n", netdev->name,
> +            netdev->port_id);
>  
>      list_push_back(&dpdk_list, &netdev->list_node);
>  
>  unlock_dev:
>      ovs_mutex_unlock(&netdev->mutex);
> -unlock_dpdk_vhost:
>      ovs_mutex_unlock(&dpdk_mutex);
>  
>      return err;
> @@ -1748,8 +1750,8 @@ new_device (struct virtio_net *dev)
>          return -1;
>      }
>  
> -    VLOG_INFO("(%ld)  VHOST Device has been added to dpdkvhost%d \n",
> -        dev->device_fh, netdev->port_id);
> +    VLOG_INFO("(%ld)  VHOST Device has been added to vhost port %s \n",
> +        dev->device_fh, netdev->name);

Thanks,

Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev