| Message ID | 1366394375.3901.36.camel@joe-AO722 |
|---|---|
| State | Accepted |
| Commit | a1c06609b37c304c58f6d1d3b8e987edaf3391f3 |
| Headers | show |
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..."
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.
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
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)
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
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.
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.
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!
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
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); }
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(-)