Re: [dpdk-dev] [PATCH 1/2] lib/librte_vhost: vhost library support to facilitate integration with vswitch.

["Xie, Huawei" <huawei.xie@intel.com>]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, August 08, 2014 1:59 AM
> To: Xie, Huawei
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] lib/librte_vhost: vhost library support to
> facilitate integration with vswitch.
> 
> On Fri, 18 Jul 2014 13:39:05 +0800
> Huawei Xie <huawei.xie@intel.com> wrote:
> 
> > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > Acked-by: Thomos Long <thomas.long@intel.com>
> 
> This looks good, but there are some style convention issues:
> 
> 1. Please don't use really long lines. About 100 or 120 characters is maximum
>    reasonable length in an editor
> 
I plan to fix the length issues in subsequent patch. There are plenty of them, which are inherited from old vhost code. 
> 2. Don't put space here in function decl.
> 
> ERROR: space prohibited after that '*' (ctx:BxW)
> #1183: FILE: lib/librte_vhost/vhost-net-cdev.h:102:
> +	int (* set_vring_kick)(struct vhost_device_ctx, struct vhost_vring_file *);
>  	     ^
> 
In this patch, I only run checkpatch.pl against the source file I modified rather than the patch itself, so missed these checking.  I had sent the V3 patch in which those kind of issues are fixed. Please refer to the latest V3 patch.
> 3. Use BSD and kernel style brace
> Not:
> 
> +void
> +put_files_struct (struct files_struct *files)
> +{
> +	if (atomic_dec_and_test (&files->count))
> +	{
> +		BUG ();
> +	}
> +}
> 
> Instead:
> +void
> +put_files_struct (struct files_struct *files)
> +{
> +	if (atomic_dec_and_test (&files->count)) {
> +		BUG ();
> +	}
> +}
> 
> 4. All functions that are not used in other files should be marked static.
>    For example put_files_struct
> 
> 5. Switch should be indented at same level as case:
> Not:
> +	switch (ioctl)
> +	{
> +		case EVENTFD_COPY:
> +			if (copy_from_user (&eventfd_copy, argp, sizeof (struct
> eventfd_copy)))
> +				return -EFAULT;
> +
> 
> Instead:
> +	switch (ioctl) {
> +	case EVENTFD_COPY:
> +		if (copy_from_user (&eventfd_copy, argp, sizeof (struct
> eventfd_copy)))
> +			return -EFAULT

Thanks. As stated above, have fixed all the style issues except 80 length warning in v3 patch.