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

-----Original Message-----
From: Stephen Hemminger [mailto:stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org]
Sent: Friday, August 08, 2014 1:59 AM
To: Xie, Huawei
Cc: dev-VfR2kkLFssw@public.gmane.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 [off-list ref] wrote:

quoted

Signed-off-by: Huawei Xie <redacted>
Acked-by: Konstantin Ananyev <redacted>
Acked-by: Thomos Long <redacted>

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.