Messages in this thread | | | Date | Thu, 1 Apr 2021 09:35:40 +0200 | From | Daniel Wagner <> | Subject | Re: [PATCH] nvme: Export fast_io_fail_tmo to sysfs |
| |
Hi Chaitanya,
On Wed, Mar 31, 2021 at 08:34:31PM +0000, Chaitanya Kulkarni wrote: > > WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'. > > For now keep the current style.
Okay, I thought so too. I am just wondering if a patch for changing all the permission sets is acceptable. I prefer the the octal permissions ;)
> > +static ssize_t nvme_ctrl_fast_io_fail_tmo_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct nvme_ctrl *ctrl = dev_get_drvdata(dev); > > + > > + if (ctrl->opts->fast_io_fail_tmo == -1) > > + return sprintf(buf, "off\n"); > > + return sprintf(buf, "%d\n", ctrl->opts->fast_io_fail_tmo); > > do we need snprintf() for 2nd ?
Sure thing can do. Also here I followed the existing style. The other store functions tend to use sprintf().
> > + err = kstrtoint(buf, 10, &fast_io_fail_tmo); > > + if (err) > > + return -EINVAL; > > + > > since you are returning an error, you can remove next else if, this also > removes the extra line after above return. Something like this on the top > of yours totally untested :-
Right, same here, followed the existing style. I prepare a patch which addresses your comments for the existing code as well.
Thanks, Daniel
| |