diff mbox series

[v2] block: Change to use DEFINE_SHOW_ATTRIBUTE macro

Message ID 20181201142414.7766-1-tiny.windzz@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] block: Change to use DEFINE_SHOW_ATTRIBUTE macro | expand

Commit Message

Yangtao Li Dec. 1, 2018, 2:24 p.m. UTC 
Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.

Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
---
changes in v2:
-Modify some function names to avoid compilation errors
---
 drivers/block/aoe/aoeblk.c        | 16 +++-----------
 drivers/block/drbd/drbd_debugfs.c | 13 +----------
 drivers/block/nbd.c               | 28 ++++--------------------
 drivers/block/pktcdvd.c           | 36 +++++++++++--------------------
 drivers/block/rsxx/core.c         | 35 ++++++------------------------
 5 files changed, 26 insertions(+), 102 deletions(-)

Comments

Jens Axboe Dec. 1, 2018, 6:10 p.m. UTC | #1 
On 12/1/18 7:24 AM, Yangtao Li wrote:
> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
> 
> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> ---
> changes in v2:
> -Modify some function names to avoid compilation errors

The fact that your previous patch didn't even compile doesn't fill me
with a lot of confidence in the amount of diligence and testing
you apply to your patches.

Why would you send something out that you didn't even compile?
Yangtao Li Dec. 1, 2018, 6:31 p.m. UTC | #2 
On Sun, Dec 2, 2018 at 2:11 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 12/1/18 7:24 AM, Yangtao Li wrote:
> > Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
> >
> > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> > ---
> > changes in v2:
> > -Modify some function names to avoid compilation errors
>
> The fact that your previous patch didn't even compile doesn't fill me
> with a lot of confidence in the amount of diligence and testing
> you apply to your patches.
>
> Why would you send something out that you didn't even compile?
>
> --
> Jens Axboe
>
These changes are the same and only require a small change.
Most of the changes are fine, so it's a bit negligent.

MBR,
Yangtao
Jens Axboe Dec. 1, 2018, 6:38 p.m. UTC | #3 
On 12/1/18 11:31 AM, Frank Lee wrote:
> On Sun, Dec 2, 2018 at 2:11 AM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 12/1/18 7:24 AM, Yangtao Li wrote:
>>> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
>>>
>>> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
>>> ---
>>> changes in v2:
>>> -Modify some function names to avoid compilation errors
>>
>> The fact that your previous patch didn't even compile doesn't fill me
>> with a lot of confidence in the amount of diligence and testing
>> you apply to your patches.
>>
>> Why would you send something out that you didn't even compile?
>>
>> --
>> Jens Axboe
>>
> These changes are the same and only require a small change.
> Most of the changes are fine, so it's a bit negligent.

When someone is sending a patch for inclusion, at the very minimum
I expect it to have been compiled, and preferably tested too. Doesn't
matter how small the change is, even a one-liner should go through that.

That said, I'm not a huge fan of changes like this. It completely
hides what is going on for someone reading the code, and it's not
like there's a win on code size for example. The only win seems to
be that driver writes can't mess it up, which is a nice benefit.
Yangtao Li Dec. 1, 2018, 6:58 p.m. UTC | #4 
On Sun, Dec 2, 2018 at 2:38 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 12/1/18 11:31 AM, Frank Lee wrote:
> > On Sun, Dec 2, 2018 at 2:11 AM Jens Axboe <axboe@kernel.dk> wrote:
> >>
> >> On 12/1/18 7:24 AM, Yangtao Li wrote:
> >>> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
> >>>
> >>> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> >>> ---
> >>> changes in v2:
> >>> -Modify some function names to avoid compilation errors
> >>
> >> The fact that your previous patch didn't even compile doesn't fill me
> >> with a lot of confidence in the amount of diligence and testing
> >> you apply to your patches.
> >>
> >> Why would you send something out that you didn't even compile?
> >>
> >> --
> >> Jens Axboe
> >>
> > These changes are the same and only require a small change.
> > Most of the changes are fine, so it's a bit negligent.
>
> When someone is sending a patch for inclusion, at the very minimum
> I expect it to have been compiled, and preferably tested too. Doesn't
> matter how small the change is, even a one-liner should go through that.
>
> That said, I'm not a huge fan of changes like this. It completely
> hides what is going on for someone reading the code, and it's not
> like there's a win on code size for example. The only win seems to
> be that driver writes can't mess it up, which is a nice benefit.
>
> --
> Jens Axboe
>
Yeah,you are right.Can you review it?

Yours,
Yangtao
diff mbox series

Patch

diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
index ed26b7287256..5d2be31ac7f8 100644
--- a/drivers/block/aoe/aoeblk.c
+++ b/drivers/block/aoe/aoeblk.c
@@ -110,7 +110,7 @@  static ssize_t aoedisk_show_payload(struct device *dev,
 	return snprintf(page, PAGE_SIZE, "%lu\n", d->maxbcnt);
 }
 
-static int aoedisk_debugfs_show(struct seq_file *s, void *ignored)
+static int aoe_show(struct seq_file *s, void *ignored)
 {
 	struct aoedev *d;
 	struct aoetgt **t, **te;
@@ -154,11 +154,6 @@  static int aoedisk_debugfs_show(struct seq_file *s, void *ignored)
 	return 0;
 }
 
-static int aoe_debugfs_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, aoedisk_debugfs_show, inode->i_private);
-}
-
 static DEVICE_ATTR(state, 0444, aoedisk_show_state, NULL);
 static DEVICE_ATTR(mac, 0444, aoedisk_show_mac, NULL);
 static DEVICE_ATTR(netif, 0444, aoedisk_show_netif, NULL);
@@ -186,12 +181,7 @@  static const struct attribute_group *aoe_attr_groups[] = {
 	NULL,
 };
 
-static const struct file_operations aoe_debugfs_fops = {
-	.open = aoe_debugfs_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(aoe);
 
 static void
 aoedisk_add_debugfs(struct aoedev *d)
@@ -208,7 +198,7 @@  aoedisk_add_debugfs(struct aoedev *d)
 		p++;
 	BUG_ON(*p == '\0');
 	entry = debugfs_create_file(p, 0444, aoe_debugfs_dir, d,
-				    &aoe_debugfs_fops);
+				    &aoe_fops);
 	if (IS_ERR_OR_NULL(entry)) {
 		pr_info("aoe: cannot create debugfs file for %s\n",
 			d->gd->disk_name);
diff --git a/drivers/block/drbd/drbd_debugfs.c b/drivers/block/drbd/drbd_debugfs.c
index 5d5e8d6a8a56..e46c198c2e6a 100644
--- a/drivers/block/drbd/drbd_debugfs.c
+++ b/drivers/block/drbd/drbd_debugfs.c
@@ -892,18 +892,7 @@  static int drbd_version_show(struct seq_file *m, void *ignored)
 	return 0;
 }
 
-static int drbd_version_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, drbd_version_show, NULL);
-}
-
-static const struct file_operations drbd_version_fops = {
-	.owner = THIS_MODULE,
-	.open = drbd_version_open,
-	.llseek = seq_lseek,
-	.read = seq_read,
-	.release = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(drbd_version);
 
 /* not __exit, may be indirectly called
  * from the module-load-failure path as well. */
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 4d4d6129ff66..415473cc3b7e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1399,17 +1399,7 @@  static int nbd_dbg_tasks_show(struct seq_file *s, void *unused)
 	return 0;
 }
 
-static int nbd_dbg_tasks_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, nbd_dbg_tasks_show, inode->i_private);
-}
-
-static const struct file_operations nbd_dbg_tasks_ops = {
-	.open = nbd_dbg_tasks_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(nbd_dbg_tasks);
 
 static int nbd_dbg_flags_show(struct seq_file *s, void *unused)
 {
@@ -1434,17 +1424,7 @@  static int nbd_dbg_flags_show(struct seq_file *s, void *unused)
 	return 0;
 }
 
-static int nbd_dbg_flags_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, nbd_dbg_flags_show, inode->i_private);
-}
-
-static const struct file_operations nbd_dbg_flags_ops = {
-	.open = nbd_dbg_flags_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(nbd_dbg_flags);
 
 static int nbd_dev_dbg_init(struct nbd_device *nbd)
 {
@@ -1462,11 +1442,11 @@  static int nbd_dev_dbg_init(struct nbd_device *nbd)
 	}
 	config->dbg_dir = dir;
 
-	debugfs_create_file("tasks", 0444, dir, nbd, &nbd_dbg_tasks_ops);
+	debugfs_create_file("tasks", 0444, dir, nbd, &nbd_dbg_tasks_fops);
 	debugfs_create_u64("size_bytes", 0444, dir, &config->bytesize);
 	debugfs_create_u32("timeout", 0444, dir, &nbd->tag_set.timeout);
 	debugfs_create_u64("blocksize", 0444, dir, &config->blksize);
-	debugfs_create_file("flags", 0444, dir, nbd, &nbd_dbg_flags_ops);
+	debugfs_create_file("flags", 0444, dir, nbd, &nbd_dbg_flags_fops);
 
 	return 0;
 }
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 9381f4e3b221..2db2ac47d87d 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -106,7 +106,7 @@  static struct dentry	*pkt_debugfs_root = NULL; /* /sys/kernel/debug/pktcdvd */
 /* forward declaration */
 static int pkt_setup_dev(dev_t dev, dev_t* pkt_dev);
 static int pkt_remove_dev(dev_t pkt_dev);
-static int pkt_seq_show(struct seq_file *m, void *p);
+static int pkt_debugfs_show(struct seq_file *m, void *p);
 
 static sector_t get_zone(sector_t sector, struct pktcdvd_device *pd)
 {
@@ -452,23 +452,7 @@  static void pkt_sysfs_cleanup(void)
 
  *******************************************************************/
 
-static int pkt_debugfs_seq_show(struct seq_file *m, void *p)
-{
-	return pkt_seq_show(m, p);
-}
-
-static int pkt_debugfs_fops_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, pkt_debugfs_seq_show, inode->i_private);
-}
-
-static const struct file_operations debug_fops = {
-	.open		= pkt_debugfs_fops_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-	.owner		= THIS_MODULE,
-};
+DEFINE_SHOW_ATTRIBUTE(pkt_debugfs);
 
 static void pkt_debugfs_dev_new(struct pktcdvd_device *pd)
 {
@@ -479,7 +463,8 @@  static void pkt_debugfs_dev_new(struct pktcdvd_device *pd)
 		return;
 
 	pd->dfs_f_info = debugfs_create_file("info", 0444,
-					     pd->dfs_d_root, pd, &debug_fops);
+					     pd->dfs_d_root, pd,
+					     &pkt_debugfs_fops);
 }
 
 static void pkt_debugfs_dev_remove(struct pktcdvd_device *pd)
@@ -2501,7 +2486,7 @@  static void pkt_init_queue(struct pktcdvd_device *pd)
 	q->queuedata = pd;
 }
 
-static int pkt_seq_show(struct seq_file *m, void *p)
+static int pkt_debugfs_show(struct seq_file *m, void *p)
 {
 	struct pktcdvd_device *pd = m->private;
 	char *msg;
@@ -2617,7 +2602,7 @@  static int pkt_new_dev(struct pktcdvd_device *pd, dev_t dev)
 		goto out_mem;
 	}
 
-	proc_create_single_data(pd->name, 0, pkt_proc, pkt_seq_show, pd);
+	proc_create_single_data(pd->name, 0, pkt_proc, pkt_debugfs_show, pd);
 	pkt_dbg(1, pd, "writer mapped to %s\n", bdevname(bdev, b));
 	return 0;
 
@@ -2628,7 +2613,8 @@  static int pkt_new_dev(struct pktcdvd_device *pd, dev_t dev)
 	return ret;
 }
 
-static int pkt_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd, unsigned long arg)
+static int pkt_ioctl(struct block_device *bdev, fmode_t mode,
+		     unsigned int cmd, unsigned long arg)
 {
 	struct pktcdvd_device *pd = bdev->bd_disk->private_data;
 	int ret;
@@ -2861,7 +2847,8 @@  static void pkt_get_status(struct pkt_ctrl_command *ctrl_cmd)
 	mutex_unlock(&ctl_mutex);
 }
 
-static long pkt_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+static long pkt_ctl_ioctl(struct file *file, unsigned int cmd,
+			  unsigned long arg)
 {
 	void __user *argp = (void __user *)arg;
 	struct pkt_ctrl_command ctrl_cmd;
@@ -2899,7 +2886,8 @@  static long pkt_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 }
 
 #ifdef CONFIG_COMPAT
-static long pkt_ctl_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+static long pkt_ctl_compat_ioctl(struct file *file, unsigned int cmd,
+				  unsigned long arg)
 {
 	return pkt_ctl_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
 }
diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index 0cf4509d575c..b02886cbd0d1 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -61,7 +61,7 @@  static DEFINE_IDA(rsxx_disk_ida);
 
 /* --------------------Debugfs Setup ------------------- */
 
-static int rsxx_attr_pci_regs_show(struct seq_file *m, void *p)
+static int pci_regs_show(struct seq_file *m, void *p)
 {
 	struct rsxx_cardinfo *card = m->private;
 
@@ -123,7 +123,7 @@  static int rsxx_attr_pci_regs_show(struct seq_file *m, void *p)
 	return 0;
 }
 
-static int rsxx_attr_stats_show(struct seq_file *m, void *p)
+static int stats_show(struct seq_file *m, void *p)
 {
 	struct rsxx_cardinfo *card = m->private;
 	int i;
@@ -164,16 +164,6 @@  static int rsxx_attr_stats_show(struct seq_file *m, void *p)
 	return 0;
 }
 
-static int rsxx_attr_stats_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, rsxx_attr_stats_show, inode->i_private);
-}
-
-static int rsxx_attr_pci_regs_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, rsxx_attr_pci_regs_show, inode->i_private);
-}
-
 static ssize_t rsxx_cram_read(struct file *fp, char __user *ubuf,
 			      size_t cnt, loff_t *ppos)
 {
@@ -220,21 +210,8 @@  static const struct file_operations debugfs_cram_fops = {
 	.write		= rsxx_cram_write,
 };
 
-static const struct file_operations debugfs_stats_fops = {
-	.owner		= THIS_MODULE,
-	.open		= rsxx_attr_stats_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
-
-static const struct file_operations debugfs_pci_regs_fops = {
-	.owner		= THIS_MODULE,
-	.open		= rsxx_attr_pci_regs_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
+DEFINE_SHOW_ATTRIBUTE(stats);
+DEFINE_SHOW_ATTRIBUTE(pci_regs);
 
 static void rsxx_debugfs_dev_new(struct rsxx_cardinfo *card)
 {
@@ -248,13 +225,13 @@  static void rsxx_debugfs_dev_new(struct rsxx_cardinfo *card)
 
 	debugfs_stats = debugfs_create_file("stats", 0444,
 					    card->debugfs_dir, card,
-					    &debugfs_stats_fops);
+					    &stats_fops);
 	if (IS_ERR_OR_NULL(debugfs_stats))
 		goto failed_debugfs_stats;
 
 	debugfs_pci_regs = debugfs_create_file("pci_regs", 0444,
 					       card->debugfs_dir, card,
-					       &debugfs_pci_regs_fops);
+					       &pci_regs_fops);
 	if (IS_ERR_OR_NULL(debugfs_pci_regs))
 		goto failed_debugfs_pci_regs;