| Message ID | 20230119220809.5518-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/9] pktcdvd: Get rid of custom printing macros | expand |
From: Andy Shevchenko > Sent: 19 January 2023 22:08 > > The checkpatch.pl warns: "Prefer kstrto<type> to single variable sscanf". > Fix the code accordingly. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/block/pktcdvd.c | 34 ++++++++++++++++++---------------- > 1 file changed, 18 insertions(+), 16 deletions(-) > > diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c > index 0ec8dc8ee5ed..ad4336ae9927 100644 > --- a/drivers/block/pktcdvd.c > +++ b/drivers/block/pktcdvd.c > @@ -236,15 +236,16 @@ static ssize_t congestion_off_store(struct device *dev, > const char *buf, size_t len) > { > struct pktcdvd_device *pd = dev_get_drvdata(dev); > - int val; > + int val, ret; > > - if (sscanf(buf, "%d", &val) == 1) { > - spin_lock(&pd->lock); > - pd->write_congestion_off = val; > - init_write_congestion_marks(&pd->write_congestion_off, > - &pd->write_congestion_on); > - spin_unlock(&pd->lock); > - } > + ret = kstrtoint(buf, 10, &val); > + if (ret) > + return ret; > + > + spin_lock(&pd->lock); > + pd->write_congestion_off = val; > + init_write_congestion_marks(&pd->write_congestion_off, &pd->write_congestion_on); > + spin_unlock(&pd->lock); > return len; > } These don't look directly equivalent. The sscanf() version silently ignores trailing characters. I think kstrtoint() will generate an error. Have you actually checked that the caller allows for an error return. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Jan 19, 2023 at 10:40:12PM +0000, David Laight wrote: > From: Andy Shevchenko > > Sent: 19 January 2023 22:08 ... > > + ret = kstrtoint(buf, 10, &val); > > + if (ret) > > + return ret; > These don't look directly equivalent. > The sscanf() version silently ignores trailing characters. > I think kstrtoint() will generate an error. Yes, kstrtoint() is stricter than sscanf(), but I believe that user space not so abusive. We may always return to sscanf(), which I don't think would be good idea rather than fixing the user space. But let's see... > Have you actually checked that the caller allows for > an error return. _s_size_t somehow hints us about :-)
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c index 0ec8dc8ee5ed..ad4336ae9927 100644 --- a/drivers/block/pktcdvd.c +++ b/drivers/block/pktcdvd.c @@ -236,15 +236,16 @@ static ssize_t congestion_off_store(struct device *dev, const char *buf, size_t len) { struct pktcdvd_device *pd = dev_get_drvdata(dev); - int val; + int val, ret; - if (sscanf(buf, "%d", &val) == 1) { - spin_lock(&pd->lock); - pd->write_congestion_off = val; - init_write_congestion_marks(&pd->write_congestion_off, - &pd->write_congestion_on); - spin_unlock(&pd->lock); - } + ret = kstrtoint(buf, 10, &val); + if (ret) + return ret; + + spin_lock(&pd->lock); + pd->write_congestion_off = val; + init_write_congestion_marks(&pd->write_congestion_off, &pd->write_congestion_on); + spin_unlock(&pd->lock); return len; } static DEVICE_ATTR_RW(congestion_off); @@ -266,15 +267,16 @@ static ssize_t congestion_on_store(struct device *dev, const char *buf, size_t len) { struct pktcdvd_device *pd = dev_get_drvdata(dev); - int val; + int val, ret; - if (sscanf(buf, "%d", &val) == 1) { - spin_lock(&pd->lock); - pd->write_congestion_on = val; - init_write_congestion_marks(&pd->write_congestion_off, - &pd->write_congestion_on); - spin_unlock(&pd->lock); - } + ret = kstrtoint(buf, 10, &val); + if (ret) + return ret; + + spin_lock(&pd->lock); + pd->write_congestion_on = val; + init_write_congestion_marks(&pd->write_congestion_off, &pd->write_congestion_on); + spin_unlock(&pd->lock); return len; } static DEVICE_ATTR_RW(congestion_on);
The checkpatch.pl warns: "Prefer kstrto<type> to single variable sscanf". Fix the code accordingly. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/block/pktcdvd.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-)