[PATCH v2 15/17] Staging: quickstart: Use pr_err and pr_info for logs

[PATCH v2 15/17] Staging: quickstart: Use pr_err and pr_info for logs

[Szymon Janc]

Signed-off-by: Szymon Janc <szymon@janc.net.pl>
---
 drivers/staging/quickstart/quickstart.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/quickstart/quickstart.c b/drivers/staging/quickstart/quickstart.c
index a67b03a..2843cf7 100644
--- a/drivers/staging/quickstart/quickstart.c
+++ b/drivers/staging/quickstart/quickstart.c
@@ -25,6 +25,8 @@
 
 #define QUICKSTART_VERSION "1.03"
 
+#define pr_fmt(fmt) KBUILD_MODNAME": " fmt
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -166,6 +168,7 @@ static void quickstart_acpi_notify(acpi_handle handle, u32 event, void *data)
 		input_sync(quickstart_input);
 		break;
 	default:
+		pr_err("Unexpected ACPI event notify (%u)\n", event);
 		break;
 	}
 }
@@ -183,8 +186,7 @@ static int quickstart_acpi_ghid(struct quickstart_acpi *quickstart)
 	status = acpi_evaluate_object(quickstart->device->handle, "GHID", NULL,
 								&buffer);
 	if (ACPI_FAILURE(status)) {
-		printk(KERN_ERR "quickstart: %s GHID method failed.\n",
-						quickstart->button->name);
+		pr_err("%s GHID method failed\n", quickstart->button->name);
 		return -EINVAL;
 	}
 
@@ -207,9 +209,8 @@ static int quickstart_acpi_ghid(struct quickstart_acpi *quickstart)
 		quickstart->button->id = *(uint64_t *)buffer.pointer;
 		break;
 	default:
-		printk(KERN_ERR "quickstart: %s GHID method returned buffer "
-				"of unexpected length %u\n",
-				quickstart->button->name, buffer.length);
+		pr_err("%s GHID method returned buffer of unexpected length %u"
+				"\n", quickstart->button->name, buffer.length);
 		ret = -EINVAL;
 		break;
 	}
@@ -269,7 +270,7 @@ static int quickstart_acpi_add(struct acpi_device *device)
 						quickstart_acpi_notify,
 						quickstart);
 	if (ACPI_FAILURE(status)) {
-		printk(KERN_ERR "quickstart: Notify handler install error\n");
+		pr_err("Notify handler install error\n");
 		ret = -ENODEV;
 		goto fail_installnotify;
 	}
@@ -309,7 +310,7 @@ static int quickstart_acpi_remove(struct acpi_device *device, int type)
 	status = acpi_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY,
 						quickstart_acpi_notify);
 	if (ACPI_FAILURE(status))
-		printk(KERN_ERR "quickstart: Error removing notify handler\n");
+		pr_err("Error removing notify handler\n");
 
 	kfree(quickstart);
 
@@ -435,8 +436,7 @@ static int __init quickstart_init(void)
 	if (ret)
 		goto fail_input;
 
-	printk(KERN_INFO "quickstart: ACPI Direct App Launch ver %s\n",
-							QUICKSTART_VERSION);
+	pr_info("ACPI Direct App Launch ver %s\n", QUICKSTART_VERSION);
 
 	return 0;
 fail_input:
-- 
1.7.8.3

Re: [PATCH v2 15/17] Staging: quickstart: Use pr_err and pr_info for logs

[Joe Perches]

On Wed, 2012-01-11 at 23:22 +0100, Szymon Janc wrote:
> Signed-off-by: Szymon Janc <szymon@janc.net.pl>

trivia:

> diff --git a/drivers/staging/quickstart/quickstart.c b/drivers/staging/quickstart/quickstart.c
[]
> @@ -25,6 +25,8 @@
[]
> +#define pr_fmt(fmt) KBUILD_MODNAME": " fmt

Add a space please between KBUILD_MODNAME and the quoted string.
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

[]
> @@ -207,9 +209,8 @@ static int quickstart_acpi_ghid(struct quickstart_acpi *quickstart)
>  		quickstart->button->id = *(uint64_t *)buffer.pointer;
>  		break;
>  	default:
> -		printk(KERN_ERR "quickstart: %s GHID method returned buffer "
> -				"of unexpected length %u\n",
> -				quickstart->button->name, buffer.length);
> +		pr_err("%s GHID method returned buffer of unexpected length %u"
> +				"\n", quickstart->button->name, buffer.length);

Please try not to break format strings into multiple bits.
It's very error prone and can make it harder to grep.
It's OK to have the line with the format exceed 80 chars.

		pr_err("%s GHID method returned buffer of unexpected length %u\n",
		       quickstart->button->name, buffer.length);

Re: [PATCH v2 15/17] Staging: quickstart: Use pr_err and pr_info for logs

[Szymon Janc]

Hi,
 
> > +#define pr_fmt(fmt) KBUILD_MODNAME": " fmt
> 
> Add a space please between KBUILD_MODNAME and the quoted string.
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Will fix that in V3. (I'll wait few more days to see if there are some more 
comments from other people)

> > @@ -207,9 +209,8 @@ static int quickstart_acpi_ghid(struct
> > quickstart_acpi *quickstart)
> > 
> >  		quickstart->button->id = *(uint64_t *)buffer.pointer;
> >  		break;
> >  	
> >  	default:
> > -		printk(KERN_ERR "quickstart: %s GHID method returned buffer "
> > -				"of unexpected length %u\n",
> > -				quickstart->button->name, buffer.length);
> > +		pr_err("%s GHID method returned buffer of unexpected length %u"
> > +				"\n", quickstart->button->name, buffer.length);
> 
> Please try not to break format strings into multiple bits.
> It's very error prone and can make it harder to grep.
> It's OK to have the line with the format exceed 80 chars.

ditto :)

-- 
Szymon K. Janc
szymon@janc.net.pl

Re: [PATCH v2 15/17] Staging: quickstart: Use pr_err and pr_info for logs

[Szymon Janc]

Hi,

> Add a space please between KBUILD_MODNAME and the quoted string.
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

> 
> Please try not to break format strings into multiple bits.
> It's very error prone and can make it harder to grep.
> It's OK to have the line with the format exceed 80 chars.
> 
> 		pr_err("%s GHID method returned buffer of unexpected length %u\n",
> 		       quickstart->button->name, buffer.length);

Both issues fixed in V3.
Since there were no other comments for V2 patches I'm sending V3 for this 
patch only.

-- 
Szymon K. Janc
szymon@janc.net.pl

Re: [PATCH v2 15/17] Staging: quickstart: Use pr_err and pr_info for logs

[Joe Perches]

On Mon, 2012-01-23 at 14:13 -0200, Jorgyano Bruno wrote:
> (an html formatted reply)

html emails are rejected by kernel.org lists,
please use text-only.

> On Wed, Jan 11, 2012 at 9:58 PM, Joe Perches <joe@perches.com> wrote:
>         It's OK to have the line with the format exceed 80 chars.
> What about the 80-columns rule?
> is the strings an exception to the rule?
> when we should not to break the strings over 80 columns?

Formats should be unbroken where possible.
80 column wrapping should be ignored for format strings.
An exception might be a multi-line format (with multiple newlines)

	printk(KERN_LEVEL "long line 1\n"
	       "long line 2\n");