[OPW,kernel,v2] Staging: xillybus: fix quoted strings coding style issue in xillybus_core.c
diff mbox

Message ID 1380910101-2844-1-git-send-email-ebru.akagunduz@gmail.com
State Changes Requested
Headers show

Commit Message

Ebru Akagündüz Oct. 4, 2013, 6:08 p.m. UTC
From: ebruAkagunduz <ebru.akagunduz@gmail.com>

This is a patch to the xillybus_core.c file that fixes quoted strings split across lines warnings found by the checkpatch.pl

Signed-off-by: Ebru Akagunduz <ebru.akagunduz@gmail.com>
---
 drivers/staging/xillybus/xillybus_core.c | 70 ++++++++++++++++----------------
 1 file changed, 35 insertions(+), 35 deletions(-)

Comments

Zach Brown Oct. 4, 2013, 6:15 p.m. UTC | #1
On Fri, Oct 04, 2013 at 09:08:21PM +0300, Ebru Akagunduz wrote:
> From: ebruAkagunduz <ebru.akagunduz@gmail.com>
> 
> This is a patch to the xillybus_core.c file that fixes quoted strings
> split across lines warnings found by the checkpatch.pl

So, in C, 

	printf("this is a "
		"long string");

Will print out "this is a long string".  The compiler merges the
adjacent string literals during compilation.

checkpatch is complaining because the single line that is being output
is built from multiple lines in the source.  It makes it hard to search
for parts of the string that happen to end up on both lines.  If you saw
the output message and searched, for example, for "a long string" you
wouldn't find it in the source.

The fix is to combine the strings in the source into one giant string so
that it's all on one line for searching.

	printf("this is a "
		"long string");

That's what Greg said at the end of the review of the last patch.

Yeah, they'll end up being enormous ugly strings, but that's the
trade-off we're willing to make so that grep -r can find them easily in
the tree.

Quoting Documentation/CodingStyle in the tree, my obnoxious underlining
added:

"
                Chapter 2: Breaking long lines and strings

Coding style is all about readability and maintainability using commonly
available tools.

The limit on the length of lines is 80 columns and this is a strongly
preferred limit.

Statements longer than 80 columns will be broken into sensible chunks,
unless exceeding 80 columns significantly increases readability and does
not hide information. Descendants are always substantially shorter than
the parent and are placed substantially to the right. The same applies
to function headers with a long argument list. ___However, never break
user-visible strings such as printk messages, because that breaks the
ability to grep for them___.
"

> -	pr_warn("xillybus: Malformed message (skipping): "
> +	pr_warn("xillybus: Malformed message (skipping):\n"
>  		"opcode=%d, channel=%03x, dir=%d, bufno=%03x, data=%07x\n",
>  		opcode, msg_channel, msg_dir, msg_bufno, msg_data);

So don't add a newline, changing the message, combine the strings into
one:

	pr_warn("xillybus: Malformed message (skipping): opcode=%d, channel=%03x, dir=%d, bufno=%03x, data=%07x\n",
  		opcode, msg_channel, msg_dir, msg_bufno, msg_data);

Et voila.

- z
Zach Brown Oct. 4, 2013, 6:17 p.m. UTC | #2
> The fix is to combine the strings in the source into one giant string so
> that it's all on one line for searching.
> 
> 	printf("this is a "
> 		"long string");

(er, uh, this fixed version should have been

 	printf("this is a long string");

obviously.  I just had my second morning coffee, so I'm moving a bit
toooo fast :)).

- z
Josh Triplett Oct. 4, 2013, 7:09 p.m. UTC | #3
On Fri, Oct 04, 2013 at 09:08:21PM +0300, Ebru Akagunduz wrote:
> From: ebruAkagunduz <ebru.akagunduz@gmail.com>
> 
> This is a patch to the xillybus_core.c file that fixes quoted strings split across lines warnings found by the checkpatch.pl

In addition to Zach's comments about how to join strings, a few minor
issues with the commit message.

First, you shouldn't have the additional "From" line above; that's only
needed when sending a patch written by someone else.  If git is
automatically adding that line, it means either git, git format-patch,
or git-send-email is misconfigured to think your name is "ebruAkagunduz"
rather than "Ebru Akagunduz".

Second, you should word-wrap any non-code lines in your comment message.

Finally, you shouldn't write "This is a patch to" or similar; your
commit message should be written imperatively, like this:

Fix checkpatch.pl issues with quoted strings split across lines in
xillybys_core.c:
(paste exact output from checkpatch.pl here)

- Josh Triplett
Sarah Sharp Oct. 7, 2013, 3:44 p.m. UTC | #4
On Fri, Oct 04, 2013 at 12:09:35PM -0700, Josh Triplett wrote:
> On Fri, Oct 04, 2013 at 09:08:21PM +0300, Ebru Akagunduz wrote:
> > From: ebruAkagunduz <ebru.akagunduz@gmail.com>
> > 
> > This is a patch to the xillybus_core.c file that fixes quoted strings split across lines warnings found by the checkpatch.pl
> 
> In addition to Zach's comments about how to join strings, a few minor
> issues with the commit message.
> 
> First, you shouldn't have the additional "From" line above; that's only
> needed when sending a patch written by someone else.  If git is
> automatically adding that line, it means either git, git format-patch,
> or git-send-email is misconfigured to think your name is "ebruAkagunduz"
> rather than "Ebru Akagunduz".
> 
> Second, you should word-wrap any non-code lines in your comment message.
> 
> Finally, you shouldn't write "This is a patch to" or similar; your
> commit message should be written imperatively, like this:
> 
> Fix checkpatch.pl issues with quoted strings split across lines in
> xillybys_core.c:
> (paste exact output from checkpatch.pl here)

Josh, do you want to add this advice to PatchPhilosophy?  Several
applicants have run into this.

Sarah Sharp

Patch
diff mbox

diff --git a/drivers/staging/xillybus/xillybus_core.c b/drivers/staging/xillybus/xillybus_core.c
index 7db6f03..ff85177 100644
--- a/drivers/staging/xillybus/xillybus_core.c
+++ b/drivers/staging/xillybus/xillybus_core.c
@@ -112,7 +112,7 @@  static void malformed_message(u32 *buf)
 	msg_bufno = (buf[0] >> 12) & 0x3ff;
 	msg_data = buf[1] & 0xfffffff;
 
-	pr_warn("xillybus: Malformed message (skipping): "
+	pr_warn("xillybus: Malformed message (skipping):\n"
 		"opcode=%d, channel=%03x, dir=%d, bufno=%03x, data=%07x\n",
 		opcode, msg_channel, msg_dir, msg_bufno, msg_data);
 }
@@ -153,14 +153,14 @@  irqreturn_t xillybus_isr(int irq, void *data)
 	for (i = 0; i < buf_size; i += 2)
 		if (((buf[i+1] >> 28) & 0xf) != ep->msg_counter) {
 			malformed_message(&buf[i]);
-			pr_warn("xillybus: Sending a NACK on "
+			pr_warn("xillybus: Sending a NACK on\n"
 				"counter %x (instead of %x) on entry %d\n",
 				((buf[i+1] >> 28) & 0xf),
 				ep->msg_counter,
 				i/2);
 
 			if (++ep->failed_messages > 10)
-				pr_err("xillybus: Lost sync with "
+				pr_err("xillybus: Lost sync with\n"
 				       "interrupt messages. Stopping.\n");
 			else {
 				ep->ephw->hw_sync_sgl_for_device(
@@ -283,12 +283,12 @@  irqreturn_t xillybus_isr(int irq, void *data)
 		case XILLYMSG_OPCODE_FATAL_ERROR:
 			ep->fatal_error = 1;
 			wake_up_interruptible(&ep->ep_wait); /* For select() */
-			pr_err("xillybus: FPGA reported a fatal "
-			       "error. This means that the low-level "
-			       "communication with the device has failed. "
-			       "This hardware problem is most likely "
-			       "unrelated to xillybus (neither kernel "
-			       "module nor FPGA core), but reports are "
+			pr_err("xillybus: FPGA reported a fatal\n"
+			       "error. This means that the low-level\n"
+			       "communication with the device has failed.\n"
+			       "This hardware problem is most likely\n"
+			       "unrelated to xillybus (neither kernel\n"
+			       "module nor FPGA core), but reports are\n"
 			       "still welcome. All I/O is aborted.\n");
 			break;
 		default:
@@ -486,7 +486,7 @@  static int xilly_setupchannels(struct xilly_endpoint *ep,
 
 		if ((channelnum > ep->num_channels) ||
 		    ((channelnum == 0) && !is_writebuf)) {
-			pr_err("xillybus: IDT requests channel out "
+			pr_err("xillybus: IDT requests channel out\n"
 			       "of range. Aborting.\n");
 			return -ENODEV;
 		}
@@ -565,8 +565,8 @@  static int xilly_setupchannels(struct xilly_endpoint *ep,
 				 */
 				if ((left_of_wr_salami < bytebufsize) &&
 				    (left_of_wr_salami > 0)) {
-					pr_err("xillybus: "
-					       "Corrupt buffer allocation "
+					pr_err("xillybus:\n"
+					       "Corrupt buffer allocation\n"
 					       "in IDT. Aborting.\n");
 					return -ENODEV;
 				}
@@ -644,8 +644,8 @@  static int xilly_setupchannels(struct xilly_endpoint *ep,
 				 */
 				if ((left_of_rd_salami < bytebufsize) &&
 				    (left_of_rd_salami > 0)) {
-					pr_err("xillybus: "
-					       "Corrupt buffer allocation "
+					pr_err("xillybus:\n"
+					       "Corrupt buffer allocation\n"
 					       "in IDT. Aborting.\n");
 					return -ENODEV;
 				}
@@ -706,7 +706,7 @@  static int xilly_setupchannels(struct xilly_endpoint *ep,
 	}
 
 	if (!msg_buf_done) {
-		pr_err("xillybus: Corrupt IDT: No message buffer. "
+		pr_err("xillybus: Corrupt IDT: No message buffer.\n"
 		       "Aborting.\n");
 		return -ENODEV;
 	}
@@ -714,7 +714,7 @@  static int xilly_setupchannels(struct xilly_endpoint *ep,
 	return 0;
 
 memfail:
-	pr_err("xillybus: Failed to allocate write buffer memory. "
+	pr_err("xillybus: Failed to allocate write buffer memory.\n"
 	       "Aborting.\n");
 	return -ENOMEM;
 dmafail:
@@ -745,7 +745,7 @@  static void xilly_scan_idt(struct xilly_endpoint *endpoint,
 	scan++;
 
 	if (scan > end_of_idt) {
-		pr_err("xillybus: IDT device name list overflow. "
+		pr_err("xillybus: IDT device name list overflow.\n"
 		       "Aborting.\n");
 		idt_handle->chandesc = NULL;
 		return;
@@ -757,7 +757,7 @@  static void xilly_scan_idt(struct xilly_endpoint *endpoint,
 	if (len & 0x03) {
 		idt_handle->chandesc = NULL;
 
-		pr_err("xillybus: Corrupt IDT device name list. "
+		pr_err("xillybus: Corrupt IDT device name list.\n"
 		       "Aborting.\n");
 	}
 
@@ -803,7 +803,7 @@  static int xilly_obtain_idt(struct xilly_endpoint *endpoint)
 		DMA_FROM_DEVICE);
 
 	if (channel->wr_buffers[0]->end_offset != endpoint->idtlen) {
-		pr_err("xillybus: IDT length mismatch (%d != %d). "
+		pr_err("xillybus: IDT length mismatch (%d != %d).\n"
 		       "Aborting.\n",
 		       channel->wr_buffers[0]->end_offset, endpoint->idtlen);
 		rc = -ENODEV;
@@ -821,8 +821,8 @@  static int xilly_obtain_idt(struct xilly_endpoint *endpoint)
 
 	/* Check version number. Accept anything below 0x82 for now. */
 	if (*version > 0x82) {
-		pr_err("xillybus: No support for IDT version 0x%02x. "
-		       "Maybe the xillybus driver needs an upgarde. "
+		pr_err("xillybus: No support for IDT version 0x%02x.\n"
+		       "Maybe the xillybus driver needs an upgarde.\n"
 		       "Aborting.\n",
 		       (int) *version);
 		rc = -ENODEV;
@@ -1312,8 +1312,8 @@  static int xillybus_myflush(struct xilly_channel *channel, long timeout)
 				 channel->rd_wait,
 				 (!channel->rd_full),
 				 timeout) == 0) {
-			pr_warn("xillybus: "
-				"Timed out while flushing. "
+			pr_warn("xillybus:\n"
+				"Timed out while flushing.\n"
 				"Output data may be lost.\n");
 
 			rc = -ETIMEDOUT;
@@ -1354,10 +1354,10 @@  static void xillybus_autoflush(struct work_struct *work)
 	rc = xillybus_myflush(channel, -1);
 
 	if (rc == -EINTR)
-		pr_warn("xillybus: Autoflush failed because "
+		pr_warn("xillybus: Autoflush failed because\n"
 			"work queue thread got a signal.\n");
 	else if (rc)
-		pr_err("xillybus: Autoflush failed under "
+		pr_err("xillybus: Autoflush failed under\n"
 		       "weird circumstances.\n");
 
 }
@@ -1615,7 +1615,7 @@  static int xillybus_open(struct inode *inode, struct file *filp)
 	mutex_unlock(&ep_list_lock);
 
 	if (!endpoint) {
-		pr_err("xillybus: open() failed to find a device "
+		pr_err("xillybus: open() failed to find a device\n"
 		       "for major=%d and minor=%d\n", major, minor);
 		return -ENODEV;
 	}
@@ -1642,14 +1642,14 @@  static int xillybus_open(struct inode *inode, struct file *filp)
 	if ((filp->f_mode & FMODE_READ) && (filp->f_flags & O_NONBLOCK) &&
 	    (channel->wr_synchronous || !channel->wr_allow_partial ||
 	     !channel->wr_supports_nonempty)) {
-		pr_err("xillybus: open() failed: "
+		pr_err("xillybus: open() failed:\n"
 		       "O_NONBLOCK not allowed for read on this device\n");
 		return -ENODEV;
 	}
 
 	if ((filp->f_mode & FMODE_WRITE) && (filp->f_flags & O_NONBLOCK) &&
 	    (channel->rd_synchronous || !channel->rd_allow_partial)) {
-		pr_err("xillybus: open() failed: "
+		pr_err("xillybus: open() failed:\n"
 		       "O_NONBLOCK not allowed for write on this device\n");
 		return -ENODEV;
 	}
@@ -1765,7 +1765,7 @@  static int xillybus_release(struct inode *inode, struct file *filp)
 		rc = mutex_lock_interruptible(&channel->rd_mutex);
 
 		if (rc) {
-			pr_warn("xillybus: Failed to close file. "
+			pr_warn("xillybus: Failed to close file.\n"
 				"Hardware left in messy state.\n");
 			return rc;
 		}
@@ -1791,7 +1791,7 @@  static int xillybus_release(struct inode *inode, struct file *filp)
 	if (filp->f_mode & FMODE_READ) {
 		rc = mutex_lock_interruptible(&channel->wr_mutex);
 		if (rc) {
-			pr_warn("xillybus: Failed to close file. "
+			pr_warn("xillybus: Failed to close file.\n"
 				"Hardware left in messy state.\n");
 			return rc;
 		}
@@ -1853,9 +1853,9 @@  static int xillybus_release(struct inode *inode, struct file *filp)
 
 				if (channel->wr_sleepy) {
 					mutex_unlock(&channel->wr_mutex);
-					pr_warn("xillybus: Hardware failed to "
-						"respond to close command, "
-						"therefore left in "
+					pr_warn("xillybus: Hardware failed to\n"
+						"respond to close command,\n"
+						"therefore left in\n"
 						"messy state.\n");
 					return -EINTR;
 				}
@@ -2057,7 +2057,7 @@  static int xillybus_init_chrdev(struct xilly_endpoint *endpoint,
 				       "%s", devname);
 
 		if (IS_ERR(device)) {
-			pr_warn("xillybus: Failed to create %s "
+			pr_warn("xillybus: Failed to create %s\n"
 				"device. Aborting.\n", devname);
 			goto error3;
 		}
@@ -2141,7 +2141,7 @@  static int xilly_quiesce(struct xilly_endpoint *endpoint)
 					 XILLY_TIMEOUT);
 
 	if (endpoint->idtlen < 0) {
-		pr_err("xillybus: Failed to quiesce the device on "
+		pr_err("xillybus: Failed to quiesce the device on\n"
 		       "exit. Quitting while leaving a mess.\n");
 		return -ENODEV;
 	}