Re: [PATCH] s390: replace strict_strto() with kstrto()

Steffen Maier <maier@xxxxxxxxxxxxxxxxxx> · Fri, 19 Jul 2013 16:43:34 +0200

Hi, it would have been nice to also CC all affected driver maintainers. 
I only saw this by accident and noticed that it might collide with my 
patch queue. Even more so since much of your patch touches code which 
usually goes through different upstream contributors than the core s390 
architecture code.

On 07/19/2013 10:48 AM, Heiko Carstens wrote:
On Fri, Jul 19, 2013 at 04:21:28PM +0900, Jingoo Han wrote:
The usage of strict_strtoul() and strict_strtol() is not preferred,
because strict_strtoul() and strict_strtol() are obsolete. Thus,
kstrtoul() and kstrtol() should be used.

Signed-off-by: Jingoo Han <jg1.han@xxxxxxxxxxx>

Nice cleanup, however I'm not going to apply this one:

@@ -1220,13 +1224,15 @@ dasd_expires_store(struct device *dev, struct device_attribute *attr,
  {
  	struct dasd_device *device;
  	unsigned long val;
+	int err;

  	device = dasd_device_from_cdev(to_ccwdev(dev));
  	if (IS_ERR(device))
  		return -ENODEV;
-
-	if ((strict_strtoul(buf, 10, &val) != 0) ||
-	    (val > DASD_EXPIRES_MAX) || val == 0) {
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+	if ((val > DASD_EXPIRES_MAX) || val == 0)
  		dasd_put_device(device);
  		return -EINVAL;
  	}

-ECOMPILE (missing opening brace at if statement).

@@ -1302,13 +1314,17 @@ dasd_timeout_store(struct device *dev, struct device_attribute *attr,
  	struct dasd_device *device;
  	struct request_queue *q;
  	unsigned long val, flags;
+	int err;

  	device = dasd_device_from_cdev(to_ccwdev(dev));
  	if (IS_ERR(device) || !device->block)
  		return -ENODEV;

-	if ((strict_strtoul(buf, 10, &val) != 0) ||
-	    val > UINT_MAX / HZ) {
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
+

Missing dasd_put_device().

diff --git a/drivers/s390/cio/ccwgroup.c b/drivers/s390/cio/ccwgroup.c
index 84846c2..959135a 100644
--- a/drivers/s390/cio/ccwgroup.c
+++ b/drivers/s390/cio/ccwgroup.c
@@ -137,7 +137,7 @@ static ssize_t ccwgroup_online_store(struct device *dev,
  	if (!try_module_get(gdrv->driver.owner))
  		return -EINVAL;

-	ret = strict_strtoul(buf, 0, &value);
+	ret = kstrtoul(buf, 0, &value);
  	if (ret)
  		goto out;

If all conversions were like this one, I would apply your patch.

--- a/drivers/s390/net/qeth_l3_sys.c
+++ b/drivers/s390/net/qeth_l3_sys.c
@@ -208,11 +208,10 @@ static ssize_t qeth_l3_dev_sniffer_store(struct device *dev,
  		goto out;
  	}

-	rc = strict_strtoul(buf, 16, &i);
-	if (rc) {
-		rc = -EINVAL;
+	rc = kstrtoul(buf, 16, &i);
+	if (rc)
  		goto out;

This hunk e.g. changes the behaviour of a user space interface. Before
your patch -ERANGE was mapped to -EINVAL. Now it is not true anymore.
This may or may not be a problem, however I'd like to keep the behaviour
as is in order to not risk any user space breakage.

So if you could resend a new version which just mechanically replaces
strict_strtoul() witch kstrtoul() that would be great.

For zfcp we have exactly such a patch from Martin in my zfcp upstream 
queue which I would send to James in the near future.

--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on System z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html









Re: [PATCH] s390: replace strict_strto*() with kstrto*()

Re: [PATCH] s390: replace strict_strto() with kstrto()