[dpdk-dev] [PATCH 1/2] lib/librte_vhost: vhost library support to facilitate integration with vswitch.
Xie, Huawei
huawei.xie at intel.com
Fri Aug 8 07:51:44 CEST 2014
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Friday, August 08, 2014 1:59 AM
> To: Xie, Huawei
> Cc: dev at 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 at intel.com> wrote:
>
> > Signed-off-by: Huawei Xie <huawei.xie at intel.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev at intel.com>
> > Acked-by: Thomos Long <thomas.long at 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.
More information about the dev
mailing list