| Message ID | 20220917071427.28499-1-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-v2] lib/notifier-error-inject: fix error when writing errno to debugfs file | expand |
On 17.09.22 09:14, Akinobu Mita wrote: > The simple attribute files do not accept a negative value since the > commit 488dac0c9237 ("libfs: fix error cast of negative value in > simple_attr_write()"), so we can no longer use DEFINE_SIMPLE_ATTRIBUTE() to > define a file operations for errno value. > > Fixes: 488dac0c9237 ("libfs: fix error cast of negative value in simple_attr_write()") > Reported-by: Zhao Gongyi <zhaogongyi@huawei.com> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> But shouldn't we fix simple_attr_write() instead? I mean, simple_attr_read() might use attr->fmt to print a signed value, but simple_attr_write() fails on signed values now? I might be wrong, but there is a disconnect. I feel like simple_attr_write() should similarly make decisions based on attr->fmt. > --- > v2: Fix Reported-by line > > lib/notifier-error-inject.c | 38 ++++++++++++++++++++++++++++++------- > 1 file changed, 31 insertions(+), 7 deletions(-) > > diff --git a/lib/notifier-error-inject.c b/lib/notifier-error-inject.c > index 21016b32d313..30ec41f58d53 100644 > --- a/lib/notifier-error-inject.c > +++ b/lib/notifier-error-inject.c > @@ -3,20 +3,44 @@ > > #include "notifier-error-inject.h" > > -static int debugfs_errno_set(void *data, u64 val) > +static int notifier_err_errno_show(struct seq_file *m, void *data) > { > - *(int *)data = clamp_t(int, val, -MAX_ERRNO, 0); > + int *value = m->private; > + > + seq_printf(m, "%d\n", *value); > + > return 0; > } > > -static int debugfs_errno_get(void *data, u64 *val) > +static int notifier_err_errno_open(struct inode *inode, struct file *file) > { > - *val = *(int *)data; > - return 0; > + return single_open(file, notifier_err_errno_show, inode->i_private); > +} > + > +static ssize_t notifier_err_errno_write(struct file *file, const char __user *ubuf, size_t len, > + loff_t *offp) > +{ > + struct seq_file *m = file->private_data; > + int *value = m->private; > + int ret; > + > + ret = kstrtoint_from_user(ubuf, len, 0, value); > + if (ret) > + return ret; > + > + *value = clamp(*value, -MAX_ERRNO, 0); > + > + return len; > } > > -DEFINE_SIMPLE_ATTRIBUTE(fops_errno, debugfs_errno_get, debugfs_errno_set, > - "%lld\n"); > +static const struct file_operations fops_errno = { > + .owner = THIS_MODULE, > + .open = notifier_err_errno_open, > + .read = seq_read, > + .write = notifier_err_errno_write, > + .llseek = seq_lseek, > + .release = single_release, > +}; > > static struct dentry *debugfs_create_errno(const char *name, umode_t mode, > struct dentry *parent, int *value)
2022年9月19日(月) 18:20 David Hildenbrand <david@redhat.com>: > > On 17.09.22 09:14, Akinobu Mita wrote: > > The simple attribute files do not accept a negative value since the > > commit 488dac0c9237 ("libfs: fix error cast of negative value in > > simple_attr_write()"), so we can no longer use DEFINE_SIMPLE_ATTRIBUTE() to > > define a file operations for errno value. > > > > Fixes: 488dac0c9237 ("libfs: fix error cast of negative value in simple_attr_write()") > > Reported-by: Zhao Gongyi <zhaogongyi@huawei.com> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > > But shouldn't we fix simple_attr_write() instead? > > I mean, simple_attr_read() might use attr->fmt to print a signed value, > but simple_attr_write() fails on signed values now? > > I might be wrong, but there is a disconnect. I feel like > simple_attr_write() should similarly make decisions based on attr->fmt. I agree there is a disconnect, but I have no idea how to fix simple_attr_write(). (strcmp(attr->fmt, "%%lld\n") is ugly) If no one seems to come up with a good idea, I'll fix the similar problems in fault-injection the same way I did here.
On 19.09.22 13:40, Akinobu Mita wrote: > 2022年9月19日(月) 18:20 David Hildenbrand <david@redhat.com>: >> >> On 17.09.22 09:14, Akinobu Mita wrote: >>> The simple attribute files do not accept a negative value since the >>> commit 488dac0c9237 ("libfs: fix error cast of negative value in >>> simple_attr_write()"), so we can no longer use DEFINE_SIMPLE_ATTRIBUTE() to >>> define a file operations for errno value. >>> >>> Fixes: 488dac0c9237 ("libfs: fix error cast of negative value in simple_attr_write()") >>> Reported-by: Zhao Gongyi <zhaogongyi@huawei.com> >>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> >> >> But shouldn't we fix simple_attr_write() instead? >> >> I mean, simple_attr_read() might use attr->fmt to print a signed value, >> but simple_attr_write() fails on signed values now? >> >> I might be wrong, but there is a disconnect. I feel like >> simple_attr_write() should similarly make decisions based on attr->fmt. > > I agree there is a disconnect, but I have no idea how to fix > simple_attr_write(). > (strcmp(attr->fmt, "%%lld\n") is ugly) > If no one seems to come up with a good idea, I'll fix the similar problems > in fault-injection the same way I did here. > Maybe we simply want another interface for the handful of %lld users. Or another way to allow DEFINE_SIMPLE_ATTRIBUTE users to specify it. Might be good enough to specify instead of/in addition to "%llu" an enum that expresses what we want. $ git grep -C2 DEFINE_SIMPLE | grep "\%" tells me that we have a) %llu b) %llx c) %lld d) 0x%llx e) 0x%08llx Maybe we can adjust the debugfs cases to be more similar, to eventually get only a, b and c.
diff --git a/lib/notifier-error-inject.c b/lib/notifier-error-inject.c index 21016b32d313..30ec41f58d53 100644 --- a/lib/notifier-error-inject.c +++ b/lib/notifier-error-inject.c @@ -3,20 +3,44 @@ #include "notifier-error-inject.h" -static int debugfs_errno_set(void *data, u64 val) +static int notifier_err_errno_show(struct seq_file *m, void *data) { - *(int *)data = clamp_t(int, val, -MAX_ERRNO, 0); + int *value = m->private; + + seq_printf(m, "%d\n", *value); + return 0; } -static int debugfs_errno_get(void *data, u64 *val) +static int notifier_err_errno_open(struct inode *inode, struct file *file) { - *val = *(int *)data; - return 0; + return single_open(file, notifier_err_errno_show, inode->i_private); +} + +static ssize_t notifier_err_errno_write(struct file *file, const char __user *ubuf, size_t len, + loff_t *offp) +{ + struct seq_file *m = file->private_data; + int *value = m->private; + int ret; + + ret = kstrtoint_from_user(ubuf, len, 0, value); + if (ret) + return ret; + + *value = clamp(*value, -MAX_ERRNO, 0); + + return len; } -DEFINE_SIMPLE_ATTRIBUTE(fops_errno, debugfs_errno_get, debugfs_errno_set, - "%lld\n"); +static const struct file_operations fops_errno = { + .owner = THIS_MODULE, + .open = notifier_err_errno_open, + .read = seq_read, + .write = notifier_err_errno_write, + .llseek = seq_lseek, + .release = single_release, +}; static struct dentry *debugfs_create_errno(const char *name, umode_t mode, struct dentry *parent, int *value)
The simple attribute files do not accept a negative value since the commit 488dac0c9237 ("libfs: fix error cast of negative value in simple_attr_write()"), so we can no longer use DEFINE_SIMPLE_ATTRIBUTE() to define a file operations for errno value. Fixes: 488dac0c9237 ("libfs: fix error cast of negative value in simple_attr_write()") Reported-by: Zhao Gongyi <zhaogongyi@huawei.com> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> --- v2: Fix Reported-by line lib/notifier-error-inject.c | 38 ++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)