[PATCH v2] block: Change to use DEFINE_SHOW_ATTRIBUTE macro

[PATCH v2] block: Change to use DEFINE_SHOW_ATTRIBUTE macro

[Yangtao Li]

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(-)

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;
 
-- 
2.17.0

Re: [PATCH v2] block: Change to use DEFINE_SHOW_ATTRIBUTE macro

[Jens Axboe]

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

Re: [PATCH v2] block: Change to use DEFINE_SHOW_ATTRIBUTE macro

[Frank Lee]

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

Re: [PATCH v2] block: Change to use DEFINE_SHOW_ATTRIBUTE macro

[Jens Axboe]

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

Re: [PATCH v2] block: Change to use DEFINE_SHOW_ATTRIBUTE macro

[Frank Lee]

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