2018-12-01 14:25:16

by Frank Lee

[permalink] [raw]
Subject: [PATCH v2] block: Change to use DEFINE_SHOW_ATTRIBUTE macro

Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.

Signed-off-by: Yangtao Li <[email protected]>
---
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



2018-12-01 18:11:58

by Jens Axboe

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

On 12/1/18 7:24 AM, Yangtao Li wrote:
> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
>
> Signed-off-by: Yangtao Li <[email protected]>
> ---
> 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


2018-12-01 18:35:03

by Frank Lee

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

On Sun, Dec 2, 2018 at 2:11 AM Jens Axboe <[email protected]> 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 <[email protected]>
> > ---
> > 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

2018-12-01 18:39:47

by Jens Axboe

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

On 12/1/18 11:31 AM, Frank Lee wrote:
> On Sun, Dec 2, 2018 at 2:11 AM Jens Axboe <[email protected]> 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 <[email protected]>
>>> ---
>>> 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


2018-12-01 19:00:53

by Frank Lee

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

On Sun, Dec 2, 2018 at 2:38 AM Jens Axboe <[email protected]> wrote:
>
> On 12/1/18 11:31 AM, Frank Lee wrote:
> > On Sun, Dec 2, 2018 at 2:11 AM Jens Axboe <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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