Re: [PATCH] mtd: Convert logging messages

Re: [PATCH] mtd: Convert logging messages

[Jörn Engel]

On Fri, 19 April 2013 10:59:35 -0700, Joe Perches wrote:
>  	}
>  	list_add(&dev->list, &blkmtd_device_list);
> -	INFO("mtd%d: [%s] erase_size = %dKiB [%d]", dev->mtd.index,
> -			dev->mtd.name + strlen("block2mtd: "),
> -			dev->mtd.erasesize >> 10, dev->mtd.erasesize);
> +	pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n",
> +		dev->mtd.index,
> +		dev->mtd.name + strlen("block2mtd: "),
> +		dev->mtd.erasesize >> 10, dev->mtd.erasesize);

I personally dislike the indent-to-braces style because it causes
unnecessary churn in patches like this.  The reindenting improves
nothing at all.  On the contrary, when going through revision history
at some point in the future I have to waste brain time to verify
whether any function change has slipped in or not.  It doesn't just
waste my time right now, it will continue to waste time in the future.
It will waste time when people care about revision history because
they encounter a bug, want a fix quick and are pressed for time.

If you care about my ack, please remove random churn.  This is not a
competition about who gets the most lines in git blame.

Jörn

--
Public Domain  - Free as in Beer
General Public - Free as in Speech
BSD License    - Free as in Enterprise
Shared Source  - Free as in "Work will make you..."

[PATCH] mtd: Convert logging messages

[Joe Perches]

Use a more current logging style.

Convert homegrown ERROR/INFO macros to pr_<level>.
Convert homegrown parse_err macros to pr_err and
expand hidden flow control.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/mtd/devices/block2mtd.c | 58 ++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index e081bfe..5cb4c04 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -6,6 +6,9 @@
  *
  * Licence: GPL
  */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/blkdev.h>
@@ -18,10 +21,6 @@
 #include <linux/mount.h>
 #include <linux/slab.h>
 
-#define ERROR(fmt, args...) printk(KERN_ERR "block2mtd: " fmt "\n" , ## args)
-#define INFO(fmt, args...) printk(KERN_INFO "block2mtd: " fmt "\n" , ## args)
-
-
 /* Info for the block device */
 struct block2mtd_dev {
 	struct list_head list;
@@ -84,7 +83,7 @@ static int block2mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
 	err = _block2mtd_erase(dev, from, len);
 	mutex_unlock(&dev->write_mutex);
 	if (err) {
-		ERROR("erase failed err = %d", err);
+		pr_err("erase failed err = %d\n", err);
 		instr->state = MTD_ERASE_FAILED;
 	} else
 		instr->state = MTD_ERASE_DONE;
@@ -239,13 +238,13 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size)
 #endif
 
 	if (IS_ERR(bdev)) {
-		ERROR("error: cannot open device %s", devname);
+		pr_err("error: cannot open device %s\n", devname);
 		goto devinit_err;
 	}
 	dev->blkdev = bdev;
 
 	if (MAJOR(bdev->bd_dev) == MTD_BLOCK_MAJOR) {
-		ERROR("attempting to use an MTD device as a block device");
+		pr_err("attempting to use an MTD device as a block device\n");
 		goto devinit_err;
 	}
 
@@ -277,9 +276,10 @@ static struct block2mtd_dev *add_device(char *devname, int erase_size)
 		goto devinit_err;
 	}
 	list_add(&dev->list, &blkmtd_device_list);
-	INFO("mtd%d: [%s] erase_size = %dKiB [%d]", dev->mtd.index,
-			dev->mtd.name + strlen("block2mtd: "),
-			dev->mtd.erasesize >> 10, dev->mtd.erasesize);
+	pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n",
+		dev->mtd.index,
+		dev->mtd.name + strlen("block2mtd: "),
+		dev->mtd.erasesize >> 10, dev->mtd.erasesize);
 	return dev;
 
 devinit_err:
@@ -339,17 +339,11 @@ static inline void kill_final_newline(char *str)
 }
 
 
-#define parse_err(fmt, args...) do {	\
-	ERROR(fmt, ## args);		\
-	return 0;			\
-} while (0)
-
 #ifndef MODULE
 static int block2mtd_init_called = 0;
 static char block2mtd_paramline[80 + 12]; /* 80 for device, 12 for erase size */
 #endif
 
-
 static int block2mtd_setup2(const char *val)
 {
 	char buf[80 + 12]; /* 80 for device, 12 for erase size */
@@ -359,8 +353,10 @@ static int block2mtd_setup2(const char *val)
 	size_t erase_size = PAGE_SIZE;
 	int i, ret;
 
-	if (strnlen(val, sizeof(buf)) >= sizeof(buf))
-		parse_err("parameter too long");
+	if (strnlen(val, sizeof(buf)) >= sizeof(buf)) {
+		pr_err("parameter too long\n");
+		return 0;
+	}
 
 	strcpy(str, val);
 	kill_final_newline(str);
@@ -368,20 +364,27 @@ static int block2mtd_setup2(const char *val)
 	for (i = 0; i < 2; i++)
 		token[i] = strsep(&str, ",");
 
-	if (str)
-		parse_err("too many arguments");
+	if (str) {
+		pr_err("too many arguments\n");
+		return 0;
+	}
 
-	if (!token[0])
-		parse_err("no argument");
+	if (!token[0]) {
+		pr_err("no argument\n");
+		return 0;
+	}
 
 	name = token[0];
-	if (strlen(name) + 1 > 80)
-		parse_err("device name too long");
+	if (strlen(name) + 1 > 80) {
+		pr_err("device name too long\n");
+		return 0;
+	}
 
 	if (token[1]) {
 		ret = parse_num(&erase_size, token[1]);
 		if (ret) {
-			parse_err("illegal erase size");
+			pr_err("illegal erase size\n");
+			return 0;
 		}
 	}
 
@@ -444,8 +447,9 @@ static void block2mtd_exit(void)
 		struct block2mtd_dev *dev = list_entry(pos, typeof(*dev), list);
 		block2mtd_sync(&dev->mtd);
 		mtd_device_unregister(&dev->mtd);
-		INFO("mtd%d: [%s] removed", dev->mtd.index,
-				dev->mtd.name + strlen("block2mtd: "));
+		pr_info("mtd%d: [%s] removed\n",
+			dev->mtd.index,
+			dev->mtd.name + strlen("block2mtd: "));
 		list_del(&dev->list);
 		block2mtd_free_device(dev);
 	}

Re: [PATCH] mtd: Convert logging messages

[Joe Perches]

On Fri, 2013-04-19 at 12:55 -0400, Jörn Engel wrote:
> On Fri, 19 April 2013 10:59:35 -0700, Joe Perches wrote:
> >  	}
> >  	list_add(&dev->list, &blkmtd_device_list);
> > -	INFO("mtd%d: [%s] erase_size = %dKiB [%d]", dev->mtd.index,
> > -			dev->mtd.name + strlen("block2mtd: "),
> > -			dev->mtd.erasesize >> 10, dev->mtd.erasesize);
> > +	pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n",
> > +		dev->mtd.index,
> > +		dev->mtd.name + strlen("block2mtd: "),
> > +		dev->mtd.erasesize >> 10, dev->mtd.erasesize);
> 
> I personally dislike the indent-to-braces style because it causes
> unnecessary churn in patches like this.

It comes from an automated emacs conversion.

> The reindenting improves nothing at all.

Debatable.

> It will waste time when people care about revision history because
> they encounter a bug, want a fix quick and are pressed for time.

Not really if you use existing tools with options.

Try:
	git diff -w
	git blame -w

> If you care about my ack, please remove random churn.  This is not a
> competition about who gets the most lines in git blame.

I care not at all about commit counts,
nor things like lines added or removed.

Re: [PATCH] mtd: Convert logging messages

[Jörn Engel]

On Fri, 19 April 2013 11:27:33 -0700, Joe Perches wrote:
> On Fri, 2013-04-19 at 12:55 -0400, Jörn Engel wrote:
> > On Fri, 19 April 2013 10:59:35 -0700, Joe Perches wrote:
> > >  	}
> > >  	list_add(&dev->list, &blkmtd_device_list);
> > > -	INFO("mtd%d: [%s] erase_size = %dKiB [%d]", dev->mtd.index,
> > > -			dev->mtd.name + strlen("block2mtd: "),
> > > -			dev->mtd.erasesize >> 10, dev->mtd.erasesize);
> > > +	pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n",
> > > +		dev->mtd.index,
> > > +		dev->mtd.name + strlen("block2mtd: "),
> > > +		dev->mtd.erasesize >> 10, dev->mtd.erasesize);
> > 
> > I personally dislike the indent-to-braces style because it causes
> > unnecessary churn in patches like this.
> 
> It comes from an automated emacs conversion.

Then please teach your emacs to be a better tool.  I trust you are
smart enough to do that.

Jörn

--
He who knows that enough is enough will always have enough.
-- Lao Tsu

Re: [PATCH] mtd: Convert logging messages

[Jörn Engel]

On Tue, 23 April 2013 10:59:54 -0700, Joe Perches wrote:
> On Mon, 2013-04-22 at 10:31 -0400, Jörn Engel wrote:
> > On Fri, 19 April 2013 11:27:33 -0700, Joe Perches wrote:
> > > On Fri, 2013-04-19 at 12:55 -0400, Jörn Engel wrote:
> > > > On Fri, 19 April 2013 10:59:35 -0700, Joe Perches wrote:
> > > > >  	}
> > > > >  	list_add(&dev->list, &blkmtd_device_list);
> > > > > -	INFO("mtd%d: [%s] erase_size = %dKiB [%d]", dev->mtd.index,
> > > > > -			dev->mtd.name + strlen("block2mtd: "),
> > > > > -			dev->mtd.erasesize >> 10, dev->mtd.erasesize);
> > > > > +	pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n",
> > > > > +		dev->mtd.index,
> > > > > +		dev->mtd.name + strlen("block2mtd: "),
> > > > > +		dev->mtd.erasesize >> 10, dev->mtd.erasesize);
> > > > 
> > > > I personally dislike the indent-to-braces style because it causes
> > > > unnecessary churn in patches like this.
> > > 
> > > It comes from an automated emacs conversion.
> > 
> > Then please teach your emacs to be a better tool.
> 
> Because you personally dislike something isn't
> enough of a reason for me to change my tools nor
> enough of a reason for me to add a special
> exception to my tools for your preferences.

In that case: NAK.

Because you personally dislike something isn't
enough of a reason for me to take your patch nor
enough of a reason for me to add a special
exception to my standards for your preferences.

Cuts both way, it seems.

Jörn

--
"Translations are and will always be problematic. They inflict violence
upon two languages." (translation from German)

Re: [PATCH] mtd: Convert logging messages

[Joe Perches]

On Mon, 2013-04-22 at 10:31 -0400, Jörn Engel wrote:
> On Fri, 19 April 2013 11:27:33 -0700, Joe Perches wrote:
> > On Fri, 2013-04-19 at 12:55 -0400, Jörn Engel wrote:
> > > On Fri, 19 April 2013 10:59:35 -0700, Joe Perches wrote:
> > > >  	}
> > > >  	list_add(&dev->list, &blkmtd_device_list);
> > > > -	INFO("mtd%d: [%s] erase_size = %dKiB [%d]", dev->mtd.index,
> > > > -			dev->mtd.name + strlen("block2mtd: "),
> > > > -			dev->mtd.erasesize >> 10, dev->mtd.erasesize);
> > > > +	pr_info("mtd%d: [%s] erase_size = %dKiB [%d]\n",
> > > > +		dev->mtd.index,
> > > > +		dev->mtd.name + strlen("block2mtd: "),
> > > > +		dev->mtd.erasesize >> 10, dev->mtd.erasesize);
> > > 
> > > I personally dislike the indent-to-braces style because it causes
> > > unnecessary churn in patches like this.
> > 
> > It comes from an automated emacs conversion.
> 
> Then please teach your emacs to be a better tool.

Because you personally dislike something isn't
enough of a reason for me to change my tools nor
enough of a reason for me to add a special
exception to my tools for your preferences.

> I trust you are smart enough to do that.

Snide much?

cheers, Joe

Re: [PATCH] mtd: Convert logging messages

[Joe Perches]

On Tue, 2013-04-23 at 12:51 -0400, Jörn Engel wrote:
> NAK.

No worries.

It'd be good if you fixed the macros with hidden
flow control.

Re: [PATCH] mtd: Convert logging messages

[Artem Bityutskiy]

On Tue, 2013-04-23 at 12:51 -0400, Jörn Engel wrote:
> In that case: NAK.
> 
> Because you personally dislike something isn't
> enough of a reason for me to take your patch nor
> enough of a reason for me to add a special
> exception to my standards for your preferences.
> 
> Cuts both way, it seems.

Hi Jörn,

I think the patch is useful so I am taking it to my l2 tree. I do not
want to annoy you at all, I just think that it is not fair to drop a
tiny, but still useful cleanup just because of a bitten to death
indentation question.

David may judge himself if he wants the patch or not, but I personally
think the patch is all-right.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] mtd: Convert logging messages

[Artem Bityutskiy]

On Fri, 2013-04-19 at 10:59 -0700, Joe Perches wrote:
> Use a more current logging style.
> 
> Convert homegrown ERROR/INFO macros to pr_<level>.
> Convert homegrown parse_err macros to pr_err and
> expand hidden flow control.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Pushed to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] mtd: Convert logging messages

[Jörn Engel]

On Mon, 13 May 2013 10:58:52 +0300, Artem Bityutskiy wrote:
> 
> I think the patch is useful so I am taking it to my l2 tree. I do not
> want to annoy you at all, I just think that it is not fair to drop a
> tiny, but still useful cleanup just because of a bitten to death
> indentation question.

Fair enough.

Jörn

--
Don't worry about people stealing your ideas. If your ideas are any good,
you'll have to ram them down people's throats.
-- Howard Aiken quoted by Ken Iverson quoted by Jim Horning quoted by
   Raph Levien, 1979