Re: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and config VMDQ offload register for multicast feature

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Oct 30, 2014 at 12:49:13AM +0000, Ouyang, Changchun wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Xie, Huawei
> > Sent: Thursday, October 30, 2014 7:26 AM
> > To: Ouyang, Changchun; dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> > config VMDQ offload register for multicast feature
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang
> > Changchun
> > > Sent: Sunday, October 26, 2014 8:46 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> > > config VMDQ offload register for multicast feature
> > >
> > > This patch is to let vhost receive and forward multicast and broadcast
> > > packets, add promiscuous option into command line; and set VMDQ RX
> > mode as:
> > > ETH_VMDQ_ACCEPT_BROADCAST|ETH_VMDQ_ACCEPT_MULTICAST if
> > promisc mode is
> > > on.
> > >
> > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > ---
> > >  examples/vhost/main.c         | 25 ++++++++++++++++++++++---
> > >  lib/librte_vhost/virtio-net.c |  4 +++-
> > >  2 files changed, 25 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/examples/vhost/main.c b/examples/vhost/main.c index
> > > 291128e..c4947f7 100644
> > > --- a/examples/vhost/main.c
> > > +++ b/examples/vhost/main.c
> > > @@ -161,6 +161,9 @@
> > >  /* mask of enabled ports */
> > >  static uint32_t enabled_port_mask = 0;
> > >
> > > +/* Ports set in promiscuous mode off by default. */
> > comment is confusing
> Don't think it is confusing, 
> But I can refine it a bit.
> > > +static uint32_t promiscuous_on;
> > > +
> > >  /*Number of switching cores enabled*/  static uint32_t
> > > num_switching_cores = 0;
> > Don't initialize static variables to zero/NULL
> 
> I don't touch this line in my patch, and initialization to 0 is not an issue here. 
> 
> > >
> > > @@ -274,6 +277,7 @@ static struct rte_eth_conf vmdq_conf_default = {
> > >  			.enable_default_pool = 0,
> > >  			.default_pool = 0,
> > >  			.nb_pool_maps = 0,
> > > +			.rx_mode = 0,
> > >  			.pool_map = {{0, 0},},
> > >  		},
> > >  	},
> > Same as above, do we need to initialize static var?
> Response same as above,  no harm to initialize them to 0.

With this one, I would actually disagree. With something like this is it harder to read, since you have a whole list of values given here. The reader has to scan through the list of values to check in case any of them are non-zero. If there is no initializer, or just a single-value initializer, e.g. ... vmdq_conf_default = { .default_pool = 0 }, the user just has a single line to look at and can know that the rest of the values will be zero. Furthermore, having the initializers all spelled out like that uses up screen space, which, if the initializers weren't there would be filled instead by code which is meaningful. More meaningful code on screen again makes things easier for the reader, as they have to do less scrolling to find the context they need.

It's not a big deal either way, but that's my opinion on the matter. :-)

/Bruce