[PATCH v1 1/8] lib/string: introduce match_string() helper

[PATCH v1 1/8] lib/string: introduce match_string() helper

[Andy Shevchenko]

>From time to time we have to match a string in an array. Make a simple helper
for that purpose.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/string.h |  2 ++
 lib/string.c           | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index b0a732b..37062fb 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -131,6 +131,8 @@ extern void argv_free(char **argv);
 extern bool sysfs_streq(const char *s1, const char *s2);
 extern int strtobool(const char *s, bool *res);
 
+int match_string(const char * const *array, size_t len, const char *string);
+
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
 int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf);
diff --git a/lib/string.c b/lib/string.c
index 0323c0d..dd02270 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -631,6 +631,32 @@ bool sysfs_streq(const char *s1, const char *s2)
 EXPORT_SYMBOL(sysfs_streq);
 
 /**
+ * match_string - matches given string in an array
+ * @array:	array of strings
+ * @len:	number of strings in the array or 0 for NULL terminated arrays
+ * @string:	string to match with
+ *
+ * Return:
+ * index of a @string in the @array if matches, or %-ENODATA otherwise.
+ */
+int match_string(const char * const *array, size_t len, const char *string)
+{
+	int index = 0;
+	const char *item;
+
+	do {
+		item = array[index];
+		if (!item)
+			break;
+		if (!strcmp(item, string))
+			return index;
+	} while (++index < len || !len);
+
+	return -ENODATA;
+}
+EXPORT_SYMBOL(match_string);
+
+/**
  * strtobool - convert common user inputs into boolean values
  * @s: input string
  * @res: result
-- 
2.6.4

[PATCH v1 2/8] device property: convert to use match_string() helper

[Andy Shevchenko]

The new helper returns index of the mathing string in an array. We would use it
here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/property.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c359351..f902b55 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -647,7 +647,7 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
 	const char *propname, const char *string)
 {
 	const char **values;
-	int nval, ret, i;
+	int nval, ret;
 
 	nval = fwnode_property_read_string_array(fwnode, propname, NULL, 0);
 	if (nval < 0)
@@ -664,13 +664,7 @@ int fwnode_property_match_string(struct fwnode_handle *fwnode,
 	if (ret < 0)
 		goto out;
 
-	ret = -ENODATA;
-	for (i = 0; i < nval; i++) {
-		if (!strcmp(values[i], string)) {
-			ret = i;
-			break;
-		}
-	}
+	ret = match_string(values, nval, string);
 out:
 	kfree(values);
 	return ret;
-- 
2.6.4

Re: [PATCH v1 2/8] device property: convert to use match_string() helper

[Mika Westerberg]

On Thu, Jan 07, 2016 at 02:06:02PM +0200, Andy Shevchenko wrote:
> The new helper returns index of the mathing string in an array. We would use it
> here.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[PATCH v1 3/8] pinctrl: convert to use match_string() helper

[Andy Shevchenko]

The new helper returns index of the mathing string in an array. We would use it
here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/pinmux.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 29984b3..c223a9e 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -334,7 +334,6 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
 	unsigned num_groups;
 	int ret;
 	const char *group;
-	int i;
 
 	if (!pmxops) {
 		dev_err(pctldev->dev, "does not support mux function\n");
@@ -363,19 +362,13 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
 		return -EINVAL;
 	}
 	if (map->data.mux.group) {
-		bool found = false;
 		group = map->data.mux.group;
-		for (i = 0; i < num_groups; i++) {
-			if (!strcmp(group, groups[i])) {
-				found = true;
-				break;
-			}
-		}
-		if (!found) {
+		ret = match_string(groups, num_groups, group);
+		if (ret < 0) {
 			dev_err(pctldev->dev,
 				"invalid group \"%s\" for function \"%s\"\n",
 				group, map->data.mux.function);
-			return -EINVAL;
+			return ret;
 		}
 	} else {
 		group = groups[0];
-- 
2.6.4

Re: [PATCH v1 3/8] pinctrl: convert to use match_string() helper

[Linus Walleij]

On Thu, Jan 7, 2016 at 1:06 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The new helper returns index of the mathing string in an array. We would use it
> here.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Pretty cool ...
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 4/8] drm/edid: convert to use match_string() helper

[Andy Shevchenko]

The new helper returns index of the mathing string in an array. We would use it
here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpu/drm/drm_edid_load.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 698b8c3..9a401ae 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -170,16 +170,11 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	int i, valid_extensions = 0;
 	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
 
-	builtin = 0;
-	for (i = 0; i < GENERIC_EDIDS; i++) {
-		if (strcmp(name, generic_edid_name[i]) == 0) {
-			fwdata = generic_edid[i];
-			fwsize = sizeof(generic_edid[i]);
-			builtin = 1;
-			break;
-		}
-	}
-	if (!builtin) {
+	builtin = match_string(generic_edid_name, GENERIC_EDIDS, name);
+	if (builtin >= 0) {
+		fwdata = generic_edid[builtin];
+		fwsize = sizeof(generic_edid[builtin]);
+	} else {
 		struct platform_device *pdev;
 		int err;
 
@@ -252,7 +247,7 @@ static void *edid_load(struct drm_connector *connector, const char *name,
 	}
 
 	DRM_INFO("Got %s EDID base block and %d extension%s from "
-	    "\"%s\" for connector \"%s\"\n", builtin ? "built-in" :
+	    "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
 	    "external", valid_extensions, valid_extensions == 1 ? "" : "s",
 	    name, connector_name);
 
-- 
2.6.4

[PATCH v1 5/8] power: charger_manager: convert to use match_string() helper

[Andy Shevchenko]

The new helper returns index of the mathing string in an array. We would use it
here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/power/charger-manager.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
index 1ea5d1a..5939251 100644
--- a/drivers/power/charger-manager.c
+++ b/drivers/power/charger-manager.c
@@ -2020,27 +2020,6 @@ static void __exit charger_manager_cleanup(void)
 module_exit(charger_manager_cleanup);
 
 /**
- * find_power_supply - find the associated power_supply of charger
- * @cm: the Charger Manager representing the battery
- * @psy: pointer to instance of charger's power_supply
- */
-static bool find_power_supply(struct charger_manager *cm,
-			struct power_supply *psy)
-{
-	int i;
-	bool found = false;
-
-	for (i = 0; cm->desc->psy_charger_stat[i]; i++) {
-		if (!strcmp(psy->desc->name, cm->desc->psy_charger_stat[i])) {
-			found = true;
-			break;
-		}
-	}
-
-	return found;
-}
-
-/**
  * cm_notify_event - charger driver notify Charger Manager of charger event
  * @psy: pointer to instance of charger's power_supply
  * @type: type of charger event
@@ -2057,9 +2036,11 @@ void cm_notify_event(struct power_supply *psy, enum cm_event_types type,
 
 	mutex_lock(&cm_list_mtx);
 	list_for_each_entry(cm, &cm_list, entry) {
-		found_power_supply = find_power_supply(cm, psy);
-		if (found_power_supply)
+		if (match_string(cm->desc->psy_charger_stat, 0,
+				 psy->desc->name) >= 0) {
+			found_power_supply = true;
 			break;
+		}
 	}
 	mutex_unlock(&cm_list_mtx);
 
-- 
2.6.4

[PATCH v1 6/8] power: ab8500: convert to use match_string() helper

[Andy Shevchenko]

The new helper returns index of the mathing string in an array. We would use it
here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/power/ab8500_btemp.c    | 15 +++++----------
 drivers/power/ab8500_charger.c  | 16 +++++-----------
 drivers/power/ab8500_fg.c       | 15 +++++----------
 drivers/power/abx500_chargalg.c | 14 +++++---------
 4 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/drivers/power/ab8500_btemp.c b/drivers/power/ab8500_btemp.c
index 8f8044e..bf2e5dd 100644
--- a/drivers/power/ab8500_btemp.c
+++ b/drivers/power/ab8500_btemp.c
@@ -906,26 +906,21 @@ static int ab8500_btemp_get_property(struct power_supply *psy,
 static int ab8500_btemp_get_ext_psy_data(struct device *dev, void *data)
 {
 	struct power_supply *psy;
-	struct power_supply *ext;
+	struct power_supply *ext = dev_get_drvdata(dev);
+	const char **supplicants = (const char **)ext->supplied_to;
 	struct ab8500_btemp *di;
 	union power_supply_propval ret;
-	int i, j;
-	bool psy_found = false;
+	int j;
 
 	psy = (struct power_supply *)data;
-	ext = dev_get_drvdata(dev);
 	di = power_supply_get_drvdata(psy);
 
 	/*
 	 * For all psy where the name of your driver
 	 * appears in any supplied_to
 	 */
-	for (i = 0; i < ext->num_supplicants; i++) {
-		if (!strcmp(ext->supplied_to[i], psy->desc->name))
-			psy_found = true;
-	}
-
-	if (!psy_found)
+	j = match_string(supplicants, ext->num_supplicants, psy->desc->name);
+	if (j < 0)
 		return 0;
 
 	/* Go through all properties for the psy */
diff --git a/drivers/power/ab8500_charger.c b/drivers/power/ab8500_charger.c
index e388171..30de5d4 100644
--- a/drivers/power/ab8500_charger.c
+++ b/drivers/power/ab8500_charger.c
@@ -1929,11 +1929,11 @@ static int ab8540_charger_usb_pre_chg_enable(struct ux500_charger *charger,
 static int ab8500_charger_get_ext_psy_data(struct device *dev, void *data)
 {
 	struct power_supply *psy;
-	struct power_supply *ext;
+	struct power_supply *ext = dev_get_drvdata(dev);
+	const char **supplicants = (const char **)ext->supplied_to;
 	struct ab8500_charger *di;
 	union power_supply_propval ret;
-	int i, j;
-	bool psy_found = false;
+	int j;
 	struct ux500_charger *usb_chg;
 
 	usb_chg = (struct ux500_charger *)data;
@@ -1941,15 +1941,9 @@ static int ab8500_charger_get_ext_psy_data(struct device *dev, void *data)
 
 	di = to_ab8500_charger_usb_device_info(usb_chg);
 
-	ext = dev_get_drvdata(dev);
-
 	/* For all psy where the driver name appears in any supplied_to */
-	for (i = 0; i < ext->num_supplicants; i++) {
-		if (!strcmp(ext->supplied_to[i], psy->desc->name))
-			psy_found = true;
-	}
-
-	if (!psy_found)
+	j = match_string(supplicants, ext->num_supplicants, psy->desc->name);
+	if (j < 0)
 		return 0;
 
 	/* Go through all properties for the psy */
diff --git a/drivers/power/ab8500_fg.c b/drivers/power/ab8500_fg.c
index 3830dad..5a36cf8 100644
--- a/drivers/power/ab8500_fg.c
+++ b/drivers/power/ab8500_fg.c
@@ -2168,26 +2168,21 @@ static int ab8500_fg_get_property(struct power_supply *psy,
 static int ab8500_fg_get_ext_psy_data(struct device *dev, void *data)
 {
 	struct power_supply *psy;
-	struct power_supply *ext;
+	struct power_supply *ext = dev_get_drvdata(dev);
+	const char **supplicants = (const char **)ext->supplied_to;
 	struct ab8500_fg *di;
 	union power_supply_propval ret;
-	int i, j;
-	bool psy_found = false;
+	int j;
 
 	psy = (struct power_supply *)data;
-	ext = dev_get_drvdata(dev);
 	di = power_supply_get_drvdata(psy);
 
 	/*
 	 * For all psy where the name of your driver
 	 * appears in any supplied_to
 	 */
-	for (i = 0; i < ext->num_supplicants; i++) {
-		if (!strcmp(ext->supplied_to[i], psy->desc->name))
-			psy_found = true;
-	}
-
-	if (!psy_found)
+	j = match_string(supplicants, ext->num_supplicants, psy->desc->name);
+	if (j < 0)
 		return 0;
 
 	/* Go through all properties for the psy */
diff --git a/drivers/power/abx500_chargalg.c b/drivers/power/abx500_chargalg.c
index 541f702..d9104b1 100644
--- a/drivers/power/abx500_chargalg.c
+++ b/drivers/power/abx500_chargalg.c
@@ -975,22 +975,18 @@ static void handle_maxim_chg_curr(struct abx500_chargalg *di)
 static int abx500_chargalg_get_ext_psy_data(struct device *dev, void *data)
 {
 	struct power_supply *psy;
-	struct power_supply *ext;
+	struct power_supply *ext = dev_get_drvdata(dev);
+	const char **supplicants = (const char **)ext->supplied_to;
 	struct abx500_chargalg *di;
 	union power_supply_propval ret;
-	int i, j;
-	bool psy_found = false;
+	int j;
 	bool capacity_updated = false;
 
 	psy = (struct power_supply *)data;
-	ext = dev_get_drvdata(dev);
 	di = power_supply_get_drvdata(psy);
 	/* For all psy where the driver name appears in any supplied_to */
-	for (i = 0; i < ext->num_supplicants; i++) {
-		if (!strcmp(ext->supplied_to[i], psy->desc->name))
-			psy_found = true;
-	}
-	if (!psy_found)
+	j = match_string(supplicants, ext->num_supplicants, psy->desc->name);
+	if (j < 0)
 		return 0;
 
 	/*
-- 
2.6.4

Re: [PATCH v1 6/8] power: ab8500: convert to use match_string() helper

[Linus Walleij]

On Thu, Jan 7, 2016 at 1:06 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The new helper returns index of the mathing string in an array. We would use it
> here.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v1 7/8] ata: hpt366: convert to use match_string() helper

[Andy Shevchenko]

The new helper returns index of the mathing string in an array. We would use it
here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/ata/pata_hpt366.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/pata_hpt366.c b/drivers/ata/pata_hpt366.c
index 0038dc4..fb85cb2 100644
--- a/drivers/ata/pata_hpt366.c
+++ b/drivers/ata/pata_hpt366.c
@@ -176,17 +176,14 @@ static int hpt_dma_blacklisted(const struct ata_device *dev, char *modestr,
 			       const char * const list[])
 {
 	unsigned char model_num[ATA_ID_PROD_LEN + 1];
-	int i = 0;
+	int i;
 
 	ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
 
-	while (list[i] != NULL) {
-		if (!strcmp(list[i], model_num)) {
-			pr_warn("%s is not supported for %s\n",
-				modestr, list[i]);
-			return 1;
-		}
-		i++;
+	i = match_string(list, 0, model_num);
+	if (i >= 0) {
+		pr_warn("%s is not supported for %s\n", modestr, list[i]);
+		return 1;
 	}
 	return 0;
 }
-- 
2.6.4

Re: [PATCH v1 7/8] ata: hpt366: convert to use match_string() helper

[Tejun Heo]

On Thu, Jan 07, 2016 at 02:06:07PM +0200, Andy Shevchenko wrote:
> The new helper returns index of the mathing string in an array. We would use it
> here.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Tejun Heo <tj@kernel.org>

Please feel free to route as you see fit.

Thanks.

-- 
tejun

[PATCH v1 8/8] ide: hpt366: convert to use match_string() helper

[Andy Shevchenko]

The new helper returns index of the mathing string in an array. We would use it
here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/ide/hpt366.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/ide/hpt366.c b/drivers/ide/hpt366.c
index 696b6c1..f417332 100644
--- a/drivers/ide/hpt366.c
+++ b/drivers/ide/hpt366.c
@@ -531,14 +531,9 @@ static const struct hpt_info hpt371n = {
 	.timings	= &hpt37x_timings
 };
 
-static int check_in_drive_list(ide_drive_t *drive, const char **list)
+static bool check_in_drive_list(ide_drive_t *drive, const char **list)
 {
-	char *m = (char *)&drive->id[ATA_ID_PROD];
-
-	while (*list)
-		if (!strcmp(*list++, m))
-			return 1;
-	return 0;
+	return match_string(list, 0, (char *)&drive->id[ATA_ID_PROD]) >= 0;
 }
 
 static struct hpt_info *hpt3xx_get_info(struct device *dev)
-- 
2.6.4

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Heikki Krogerus]

On Thu, Jan 07, 2016 at 02:06:01PM +0200, Andy Shevchenko wrote:
> >From time to time we have to match a string in an array. Make a simple helper
> for that purpose.

Cool! If you make v2 out of these, could you patch the following while
at it:

diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index e6ec125..a391c81 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -64,18 +64,15 @@ EXPORT_SYMBOL_GPL(usb_speed_string);
 enum usb_device_speed usb_get_maximum_speed(struct device *dev)
 {
        const char *maximum_speed;
-       int err;
-       int i;
+       int ret;
 
-       err = device_property_read_string(dev, "maximum-speed", &maximum_speed);
-       if (err < 0)
+       ret = device_property_read_string(dev, "maximum-speed", &maximum_speed);
+       if (ret < 0)
                return USB_SPEED_UNKNOWN;
 
-       for (i = 0; i < ARRAY_SIZE(speed_names); i++)
-               if (strcmp(maximum_speed, speed_names[i]) == 0)
-                       return i;
+       ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
 
-       return USB_SPEED_UNKNOWN;
+       return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
 }
 EXPORT_SYMBOL_GPL(usb_get_maximum_speed);
 
@@ -109,13 +106,11 @@ static const char *const usb_dr_modes[] = {
 
 static enum usb_dr_mode usb_get_dr_mode_from_string(const char *str)
 {
-       int i;
+       int ret;
 
-       for (i = 0; i < ARRAY_SIZE(usb_dr_modes); i++)
-               if (!strcmp(usb_dr_modes[i], str))
-                       return i;
+       ret = match_string(usb_dr_modes, ARRAY_SIZE(usb_dr_modes), str);
 
-       return USB_DR_MODE_UNKNOWN;
+       return (ret < 0) ? USB_DR_MODE_UNKNOWN : ret;
 }
 
 enum usb_dr_mode usb_get_dr_mode(struct device *dev)


Thanks,

-- 
heikki

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Andy Shevchenko]

On Thu, 2016-01-07 at 15:07 +0200, Heikki Krogerus wrote:
> On Thu, Jan 07, 2016 at 02:06:01PM +0200, Andy Shevchenko wrote:
> > > From time to time we have to match a string in an array. Make a
> > > simple helper
> > for that purpose.
> 
> Cool! If you make v2 out of these, could you patch the following
> while
> at it:

Yes, please, format as a usual patch and share with me. I will include
in v3 (actually I missed the numbering here, it should be v2 already).

> 
> diff --git a/drivers/usb/common/common.c
> b/drivers/usb/common/common.c
> index e6ec125..a391c81 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -64,18 +64,15 @@ EXPORT_SYMBOL_GPL(usb_speed_string);
>  enum usb_device_speed usb_get_maximum_speed(struct device *dev)
>  {
>         const char *maximum_speed;
> -       int err;
> -       int i;
> +       int ret;
>  
> -       err = device_property_read_string(dev, "maximum-speed",
> &maximum_speed);
> -       if (err < 0)
> +       ret = device_property_read_string(dev, "maximum-speed",
> &maximum_speed);
> +       if (ret < 0)
>                 return USB_SPEED_UNKNOWN;
>  
> -       for (i = 0; i < ARRAY_SIZE(speed_names); i++)
> -               if (strcmp(maximum_speed, speed_names[i]) == 0)
> -                       return i;
> +       ret = match_string(speed_names, ARRAY_SIZE(speed_names),
> maximum_speed);
>  
> -       return USB_SPEED_UNKNOWN;
> +       return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
>  }
>  EXPORT_SYMBOL_GPL(usb_get_maximum_speed);
>  
> @@ -109,13 +106,11 @@ static const char *const usb_dr_modes[] = {
>  
>  static enum usb_dr_mode usb_get_dr_mode_from_string(const char *str)
>  {
> -       int i;
> +       int ret;
>  
> -       for (i = 0; i < ARRAY_SIZE(usb_dr_modes); i++)
> -               if (!strcmp(usb_dr_modes[i], str))
> -                       return i;
> +       ret = match_string(usb_dr_modes, ARRAY_SIZE(usb_dr_modes),
> str);
>  
> -       return USB_DR_MODE_UNKNOWN;
> +       return (ret < 0) ? USB_DR_MODE_UNKNOWN : ret;
>  }
>  
>  enum usb_dr_mode usb_get_dr_mode(struct device *dev)
> 
> 
> Thanks,
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

On Thu, Jan 07, 2016 at 03:12:00PM +0200, Andy Shevchenko wrote:
> On Thu, 2016-01-07 at 15:07 +0200, Heikki Krogerus wrote:
> > On Thu, Jan 07, 2016 at 02:06:01PM +0200, Andy Shevchenko wrote:
> > > > From time to time we have to match a string in an array. Make a
> > > > simple helper
> > > for that purpose.
> > 
> > Cool! If you make v2 out of these, could you patch the following
> > while
> > at it:
> 
> Yes, please, format as a usual patch and share with me. I will include
> in v3 (actually I missed the numbering here, it should be v2 already).

Sure thing. Here's the patch.

-- 
heikki

[-- Attachment #2: 0001-usb-common-convert-to-use-match_string-helper.patch --]
[-- Type: text/plain, Size: 1857 bytes --]

>From 86d8e9fda3f6a0679c9350d950f44ca2c9bec8ce Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Thu, 7 Jan 2016 15:22:14 +0200
Subject: [PATCH] usb: common: convert to use match_string() helper

The new helper returns index of the mathing string in an array. We would use it
here.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/common/common.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index e6ec125..677b3f0 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -64,18 +64,15 @@ EXPORT_SYMBOL_GPL(usb_speed_string);
 enum usb_device_speed usb_get_maximum_speed(struct device *dev)
 {
 	const char *maximum_speed;
-	int err;
-	int i;
+	int ret;
 
-	err = device_property_read_string(dev, "maximum-speed", &maximum_speed);
-	if (err < 0)
+	ret = device_property_read_string(dev, "maximum-speed", &maximum_speed);
+	if (ret < 0)
 		return USB_SPEED_UNKNOWN;
 
-	for (i = 0; i < ARRAY_SIZE(speed_names); i++)
-		if (strcmp(maximum_speed, speed_names[i]) == 0)
-			return i;
+	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
 
-	return USB_SPEED_UNKNOWN;
+	return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
 }
 EXPORT_SYMBOL_GPL(usb_get_maximum_speed);
 
@@ -109,13 +106,10 @@ static const char *const usb_dr_modes[] = {
 
 static enum usb_dr_mode usb_get_dr_mode_from_string(const char *str)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(usb_dr_modes); i++)
-		if (!strcmp(usb_dr_modes[i], str))
-			return i;
+	int ret;
 
-	return USB_DR_MODE_UNKNOWN;
+	ret = match_string(usb_dr_modes, ARRAY_SIZE(usb_dr_modes), str);
+	return (ret < 0) ? USB_DR_MODE_UNKNOWN : ret;
 }
 
 enum usb_dr_mode usb_get_dr_mode(struct device *dev)
-- 
2.6.4

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Rasmus Villemoes]

On Thu, Jan 07 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> From time to time we have to match a string in an array. Make a simple helper
> for that purpose.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/string.h |  2 ++
>  lib/string.c           | 26 ++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b0a732b..37062fb 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -131,6 +131,8 @@ extern void argv_free(char **argv);
>  extern bool sysfs_streq(const char *s1, const char *s2);
>  extern int strtobool(const char *s, bool *res);
>  
> +int match_string(const char * const *array, size_t len, const char *string);
> +
>  #ifdef CONFIG_BINARY_PRINTF
>  int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
>  int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf);
> diff --git a/lib/string.c b/lib/string.c
> index 0323c0d..dd02270 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -631,6 +631,32 @@ bool sysfs_streq(const char *s1, const char *s2)
>  EXPORT_SYMBOL(sysfs_streq);
>  
>  /**
> + * match_string - matches given string in an array
> + * @array:	array of strings
> + * @len:	number of strings in the array or 0 for NULL terminated arrays
> + * @string:	string to match with
> + *
> + * Return:
> + * index of a @string in the @array if matches, or %-ENODATA otherwise.
> + */
> +int match_string(const char * const *array, size_t len, const char *string)
> +{
> +	int index = 0;
> +	const char *item;
> +
> +	do {
> +		item = array[index];
> +		if (!item)
> +			break;
> +		if (!strcmp(item, string))
> +			return index;
> +	} while (++index < len || !len);
> +
> +	return -ENODATA;
> +}
> +EXPORT_SYMBOL(match_string);
> +

I'd suggest making it -1 (which, since len is a size_t, is effectively
infinity) having the meaning "the array is terminated by a NULL
entry". match_string(..., ARRAY_SIZE(my_array), ...) will break if the
array happens to be empty, which could e.g. happen in a case like

const char *my_array[] = {
#ifdef CONFIG_THIS
       "this",
#endif
#ifdef CONFIG_THAT
       "that",
#endif
};

I also think the condition/loop above is unreadable.

for (index = 0; index < len; index++) {
    ...
}

is much clearer.

Why -ENODATA and not just -1? It is rather unlikely that anyone would
pass on that particular -Exxx value. Not a biggie, just curious.

Would there be more potential users if we had a flag argument allowing
case-insensitive matching? Would there be more potential users if a flag
allowed to ask whether the given string is a _prefix_ of one of the
strings in the array, or vice versa? Something like

#define MATCH_STRING_CASE 0x01
#define MATCH_STRING_PREFIX_OF_ARRAY_ELEM 0x02 /* yeah, that name sucks */
#define MATCH_ARRAY_ELEM_PREFIX_OF_STRING 0x04 /* this too */

int match_string(const char * const *array, size_t len, const char *string, unsigned flags)
{
#define MATCH_PREFIX (MATCH_... | MATCH_...)
    int index;
    const char *item;
    int (*match_func)(const char *, const char *) =
        flags & MATCH_STRING_CASE ? strcasecmp : strcmp;
    int (*prefix_func)(const char *, const char *, size_t) =
        flags & MATCH_STRING_CASE ? strncasecmp : strncmp;
  
    for (index = 0; index < len; ++index) {
        item = array[index];
        if (!item)
            break;
        if (flags & MATCH_PREFIX) {
            size_t len = strlen(flags & MATCH_STRING_PREFIX_OF_ARRAY_ELEM ?
                string : item);
            if (!prefix_func(item, string, len))
                return index;
        } else if (!match_func(item, string)) {
            return index;
        }
    }
    return -1;
}

(Ok, it's not that pretty; maybe it'd be better to use
switch(flags&MATCH_PREFIX) {}. Or maybe just the case-insensitive part
is worth keeping; in that case the above isn't that bad.)

Rasmus

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Andy Shevchenko]

On Thu, 2016-01-07 at 23:05 +0100, Rasmus Villemoes wrote:
> On Thu, Jan 07 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.c
> om> wrote:
> 
> > From time to time we have to match a string in an array. Make a
> > simple helper
> > for that purpose.

> >  /**
> > + * match_string - matches given string in an array
> > + * @array:	array of strings
> > + * @len:	number of strings in the array or 0 for NULL
> > terminated arrays
> > + * @string:	string to match with
> > + *
> > + * Return:
> > + * index of a @string in the @array if matches, or %-ENODATA
> > otherwise.
> > + */
> > +int match_string(const char * const *array, size_t len, const char
> > *string)
> > +{
> > +	int index = 0;
> > +	const char *item;
> > +
> > +	do {
> > +		item = array[index];
> > +		if (!item)
> > +			break;
> > +		if (!strcmp(item, string))
> > +			return index;
> > +	} while (++index < len || !len);
> > +
> > +	return -ENODATA;
> > +}
> > +EXPORT_SYMBOL(match_string);
> > +
> 
> I'd suggest making it -1 (which, since len is a size_t, is
> effectively
> infinity) having the meaning "the array is terminated by a NULL
> entry". match_string(..., ARRAY_SIZE(my_array), ...) will break if
> the
> array happens to be empty, which could e.g. happen in a case like
> 
> const char *my_array[] = {
> #ifdef CONFIG_THIS
>        "this",
> #endif
> #ifdef CONFIG_THAT
>        "that",
> #endif
> };

It might make sense, though I don't remember current users with such
conditions.


> I also think the condition/loop above is unreadable.

Hmm… For me looks straightforward.

> 
> for (index = 0; index < len; index++) {
>     ...
> }
> 
> is much clearer.

If we switch to -1, it will look indeed simpler.

> 
> Why -ENODATA and not just -1? It is rather unlikely that anyone would
> pass on that particular -Exxx value. Not a biggie, just curious.

There are few of users already that would like to return error code to
upper level. In some cases better to have

return match_string();

than

ret = match_string();
if (ret < 0)
 return -EFOO;

return 0;

And returning -ENODATA doesn't prevent to have latter, but allows
former.

> 
> Would there be more potential users if we had a flag argument
> allowing
> case-insensitive matching? Would there be more potential users if a
> flag
> allowed to ask whether the given string is a _prefix_ of one of the
> strings in the array, or vice versa? Something like
> 
> #define MATCH_STRING_CASE 0x01
> #define MATCH_STRING_PREFIX_OF_ARRAY_ELEM 0x02 /* yeah, that name
> sucks */
> #define MATCH_ARRAY_ELEM_PREFIX_OF_STRING 0x04 /* this too */
> 
> int match_string(const char * const *array, size_t len, const char
> *string, unsigned flags)
> {
> #define MATCH_PREFIX (MATCH_... | MATCH_...)
>     int index;
>     const char *item;
>     int (*match_func)(const char *, const char *) =
>         flags & MATCH_STRING_CASE ? strcasecmp : strcmp;
>     int (*prefix_func)(const char *, const char *, size_t) =
>         flags & MATCH_STRING_CASE ? strncasecmp : strncmp;
>   
>     for (index = 0; index < len; ++index) {
>         item = array[index];
>         if (!item)
>             break;
>         if (flags & MATCH_PREFIX) {
>             size_t len = strlen(flags &
> MATCH_STRING_PREFIX_OF_ARRAY_ELEM ?
>                 string : item);
>             if (!prefix_func(item, string, len))
>                 return index;
>         } else if (!match_func(item, string)) {
>             return index;
>         }
>     }
>     return -1;
> }
> 
> (Ok, it's not that pretty; maybe it'd be better to use
> switch(flags&MATCH_PREFIX) {}. Or maybe just the case-insensitive
> part
> is worth keeping; in that case the above isn't that bad.)

I won't overcomplicate it until we have enough users to consider. Any
examples where we need this?

And I prefer way to have different prototypes for them instead of net
of conditions.

Thanks for review. I will send v3 (yeah, this is actually v2) with
change you proposed in the first part. For the second one I would like
to have real examples before doing anything.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Sergey Senozhatsky]

On (01/07/16 14:06), Andy Shevchenko wrote:
> 
> From time to time we have to match a string in an array. Make a simple helper
> for that purpose.
> 

Hello,

strncmp() case seems to be quite common.

> +int match_string(const char * const *array, size_t len, const char *string)
                                                  ^^^^^^^
a nitpick, [to me] `len' looks a bit confusing, usually it's array 'size'.

> +{
> +	int index = 0;
> +	const char *item;
> +
> +	do {
> +		item = array[index];
> +		if (!item)
> +			break;
> +		if (!strcmp(item, string))
> +			return index;
> +	} while (++index < len || !len);
> +
> +	return -ENODATA;
> +}


do you want to do something like this:

/*
 * hm, how to name this thing... nmatch_string() or match_nstring()...
 * nmatch_string() _probably_ better, match_nstring() is totally cryptic.
 */
int nmatch_string(array, array_size, string, string_len)
{
	do {
		strncmp();
	} while ();
}

int match_string(array, array_size, string)
{
	return nmatch_string(array, array_size, string, strlen(string));
}

	-ss

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Andy Shevchenko]

On Fri, 2016-01-08 at 09:13 +0900, Sergey Senozhatsky wrote:
> On (01/07/16 14:06), Andy Shevchenko wrote:
> > 
> > From time to time we have to match a string in an array. Make a
> > simple helper
> > for that purpose.
> > 
> 
> Hello,
> 
> strncmp() case seems to be quite common.

Like I answered to Rasmus, please, provide real examples.

> 
> > +int match_string(const char * const *array, size_t len, const char
> > *string)
>                                                   ^^^^^^^
> a nitpick, [to me] `len' looks a bit confusing, usually it's array
> 'size'.

Agreed. I would change it to plain 'n'.

> 
> > +{
> > +	int index = 0;
> > +	const char *item;
> > +
> > +	do {
> > +		item = array[index];
> > +		if (!item)
> > +			break;
> > +		if (!strcmp(item, string))
> > +			return index;
> > +	} while (++index < len || !len);
> > +
> > +	return -ENODATA;
> > +}
> 
> 
> do you want to do something like this:
> 
> /*
>  * hm, how to name this thing... nmatch_string() or
> match_nstring()...
>  * nmatch_string() _probably_ better, match_nstring() is totally
> cryptic.
>  */
> int nmatch_string(array, array_size, string, string_len)
> {
> 	do {
> 		strncmp();
> 	} while ();
> }
> 
> int match_string(array, array_size, string)
> {
> 	return nmatch_string(array, array_size, string,
> strlen(string));
> }

See above.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Sergey Senozhatsky]

Andy Shevchenko wrote:
[..]
> >
> > strncmp() case seems to be quite common.
> 
> Like I answered to Rasmus, please, provide real examples.

[..]
> > int nmatch_string(array, array_size, string, string_len)
> > {
> >       do {
> >               strncmp();
> >       } while ();
> > }
> >
> > int match_string(array, array_size, string)
> > {
> >       return nmatch_string(array, array_size, string,
> > strlen(string));
> > }
> 
> See above.

after some quick and inaccurate grepping, well, probably you're right - not worth it.

arch/mips/bcm63xx/boards/board_bcm963xx.c  void __init board_prom_init(void)
net/irda/irnet/irnet_irda.c   irnet_dname_to_daddr()
arch/powerpc/sysdev/ppc4xx_cpm.c static ssize_t cpm_idle_store()
arch/x86/ras/mce_amd_inj.c static int __set_inj
drivers/hwtracing/intel_th/msu.c mode_store
drivers/pci/pcie/aer/ecrc.c void pcie_ecrc_get_policy
drivers/pci/pcie/aspm.c pcie_aspm_set_policy
drivers/scsi/aic7xxx/aic7xxx_osm.c aic7xxx_setup
drivers/scsi/aic7xxx/aic79xx_osm.c aic79xx_setup
drivers/scsi/scsi_transport_fc.c static int get_fc_##title##_match
drivers/staging/android/ion/hisilicon/hi6220_ion.c get_type_by_name
drivers/staging/lustre/lustre/lmv/lproc_lmv.c placement_name2policy
drivers/xen/sys-hypervisor.c pmu_mode_store

	-ss

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Andy Shevchenko]

On Sat, Jan 9, 2016 at 3:12 AM, Sergey Senozhatsky
<sergey.senozhatsky@gmail.com> wrote:
> Andy Shevchenko wrote:
> [..]
>> >
>> > strncmp() case seems to be quite common.
>>
>> Like I answered to Rasmus, please, provide real examples.
>
> [..]
>> > int nmatch_string(array, array_size, string, string_len)
>> > {
>> >       do {
>> >               strncmp();
>> >       } while ();
>> > }
>> >
>> > int match_string(array, array_size, string)
>> > {
>> >       return nmatch_string(array, array_size, string,
>> > strlen(string));
>> > }
>>
>> See above.
>
> after some quick and inaccurate grepping, well, probably you're right - not worth it.

Good grep anyway, it clearly shows that there is hard to generalize
which limit to use: a) length of a first argument / item from a list,
b) length of a second argument or a constant.

> arch/mips/bcm63xx/boards/board_bcm963xx.c  void __init board_prom_init(void)
> net/irda/irnet/irnet_irda.c   irnet_dname_to_daddr()
> arch/powerpc/sysdev/ppc4xx_cpm.c static ssize_t cpm_idle_store()
> arch/x86/ras/mce_amd_inj.c static int __set_inj
> drivers/hwtracing/intel_th/msu.c mode_store
> drivers/pci/pcie/aer/ecrc.c void pcie_ecrc_get_policy
> drivers/pci/pcie/aspm.c pcie_aspm_set_policy
> drivers/scsi/aic7xxx/aic7xxx_osm.c aic7xxx_setup
> drivers/scsi/aic7xxx/aic79xx_osm.c aic79xx_setup
> drivers/scsi/scsi_transport_fc.c static int get_fc_##title##_match
> drivers/staging/android/ion/hisilicon/hi6220_ion.c get_type_by_name
> drivers/staging/lustre/lustre/lmv/lproc_lmv.c placement_name2policy
> drivers/xen/sys-hypervisor.c pmu_mode_store

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Andy Shevchenko]

On Sat, 2016-01-09 at 13:57 +0200, Andy Shevchenko wrote:
> On Sat, Jan 9, 2016 at 3:12 AM, Sergey Senozhatsky
> <sergey.senozhatsky@gmail.com> wrote:
> > Andy Shevchenko wrote:
> > [..]
> > > > 
> > > > strncmp() case seems to be quite common.
> > > 
> > > Like I answered to Rasmus, please, provide real examples.
> > 
> > [..]
> > > > int nmatch_string(array, array_size, string, string_len)
> > > > {
> > > >       do {
> > > >               strncmp();
> > > >       } while ();
> > > > }
> > > > 
> > > > int match_string(array, array_size, string)
> > > > {
> > > >       return nmatch_string(array, array_size, string,
> > > > strlen(string));
> > > > }
> > > 
> > > See above.
> > 
> > after some quick and inaccurate grepping, well, probably you're
> > right - not worth it.
> 
> Good grep anyway, it clearly shows that there is hard to generalize
> which limit to use: a) length of a first argument / item from a list,
> b) length of a second argument or a constant.
> 
> > arch/mips/bcm63xx/boards/board_bcm963xx.c  void __init
> > board_prom_init(void)
> > net/irda/irnet/irnet_irda.c   irnet_dname_to_daddr()
> > arch/powerpc/sysdev/ppc4xx_cpm.c static ssize_t cpm_idle_store()
> > arch/x86/ras/mce_amd_inj.c static int __set_inj
> > drivers/hwtracing/intel_th/msu.c mode_store
> > drivers/pci/pcie/aer/ecrc.c void pcie_ecrc_get_policy
> > drivers/pci/pcie/aspm.c pcie_aspm_set_policy
> > drivers/scsi/aic7xxx/aic7xxx_osm.c aic7xxx_setup
> > drivers/scsi/aic7xxx/aic79xx_osm.c aic79xx_setup
> > drivers/scsi/scsi_transport_fc.c static int get_fc_##title##_match
> > drivers/staging/android/ion/hisilicon/hi6220_ion.c get_type_by_name
> > drivers/staging/lustre/lustre/lmv/lproc_lmv.c placement_name2policy
> > drivers/xen/sys-hypervisor.c pmu_mode_store

Thought more about those cases.

If you would like you may introduce something like

int nmatch_string(array, array_size, string, int len)
{
  if (len < 0)
    return match_string();

  for (…) {
    size_t itemlen = (len > 0) ? len : strlen(array[index]);
…
    if (!strncmp(array[index], string, itemlen))
      return index;
  }
  return -EINVAL;
}

And convert existing users where it makes sense.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Rasmus Villemoes]

On Mon, Jan 11 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, 2016-01-09 at 13:57 +0200, Andy Shevchenko wrote:
>> On Sat, Jan 9, 2016 at 3:12 AM, Sergey Senozhatsky
>> <sergey.senozhatsky@gmail.com> wrote:
>> > Andy Shevchenko wrote:
>> > [..]
>> > > > 
>> > > > strncmp() case seems to be quite common.
>> > > 
>> > > Like I answered to Rasmus, please, provide real examples.
>> > 
>> > [..]
>> > > > int nmatch_string(array, array_size, string, string_len)
>> > > > {
>> > > >       do {
>> > > >               strncmp();
>> > > >       } while ();
>> > > > }
>> > > > 
>> > > > int match_string(array, array_size, string)
>> > > > {
>> > > >       return nmatch_string(array, array_size, string,
>> > > > strlen(string));
>> > > > }
>> > > 
>> > > See above.
>> > 
>> > after some quick and inaccurate grepping, well, probably you're
>> > right - not worth it.
>> 
>> Good grep anyway, it clearly shows that there is hard to generalize
>> which limit to use: a) length of a first argument / item from a list,
>> b) length of a second argument or a constant.
>> 
>> > arch/mips/bcm63xx/boards/board_bcm963xx.c  void __init
>> > board_prom_init(void)
>> > net/irda/irnet/irnet_irda.c   irnet_dname_to_daddr()
>> > arch/powerpc/sysdev/ppc4xx_cpm.c static ssize_t cpm_idle_store()
>> > arch/x86/ras/mce_amd_inj.c static int __set_inj
>> > drivers/hwtracing/intel_th/msu.c mode_store
>> > drivers/pci/pcie/aer/ecrc.c void pcie_ecrc_get_policy
>> > drivers/pci/pcie/aspm.c pcie_aspm_set_policy
>> > drivers/scsi/aic7xxx/aic7xxx_osm.c aic7xxx_setup
>> > drivers/scsi/aic7xxx/aic79xx_osm.c aic79xx_setup
>> > drivers/scsi/scsi_transport_fc.c static int get_fc_##title##_match
>> > drivers/staging/android/ion/hisilicon/hi6220_ion.c get_type_by_name
>> > drivers/staging/lustre/lustre/lmv/lproc_lmv.c placement_name2policy
>> > drivers/xen/sys-hypervisor.c pmu_mode_store
>
> Thought more about those cases.
>
> If you would like you may introduce something like
>
> int nmatch_string(array, array_size, string, int len)
> {
>   if (len < 0)
>     return match_string();
>
>   for (...) {
>     size_t itemlen = (len > 0) ? len : strlen(array[index]);
> ...
>     if (!strncmp(array[index], string, itemlen))
>       return index;
>   }
>   return -EINVAL;
> }

Yeah, a separate function is probably better. But why not a more
explicit name, match_prefix, match_string_prefix, match_string_starts?

I like the idea of passing the string length if one wants the "is this a
prefix of some array element" semantics, and a sentinel otherwise. But I
don't see any case where one would want match_string() semantics (why
not call match_string directly instead?), so why not let len < 0 mean
"is some array element a prefix of this string" and "len >= 0" be the
other case. I don't see why one shouldn't be able to ask "is the empty
string a prefix of some array element" (that is, are there any elements
in the array); both the array and the string might be run-time things,
so this could occur. And it's not up to a generic library routine like
this to impose restrictions like "the empty string makes no sense, go
away".

Rasmus

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Rasmus Villemoes]

On Mon, Jan 11 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, 2016-01-09 at 13:57 +0200, Andy Shevchenko wrote:
>> On Sat, Jan 9, 2016 at 3:12 AM, Sergey Senozhatsky
>> <sergey.senozhatsky@gmail.com> wrote:
>> > Andy Shevchenko wrote:
>> > [..]
>> > > > 
>> > > > strncmp() case seems to be quite common.
>> > > 
>> > > Like I answered to Rasmus, please, provide real examples.
>> > 
>> > [..]
>> > > > int nmatch_string(array, array_size, string, string_len)
>> > > > {
>> > > >       do {
>> > > >               strncmp();
>> > > >       } while ();
>> > > > }
>> > > > 
>> > > > int match_string(array, array_size, string)
>> > > > {
>> > > >       return nmatch_string(array, array_size, string,
>> > > > strlen(string));
>> > > > }
>> > > 
>> > > See above.
>> > 
>> > after some quick and inaccurate grepping, well, probably you're
>> > right - not worth it.
>> 
>> Good grep anyway, it clearly shows that there is hard to generalize
>> which limit to use: a) length of a first argument / item from a list,
>> b) length of a second argument or a constant.
>> 
>> > arch/mips/bcm63xx/boards/board_bcm963xx.c  void __init
>> > board_prom_init(void)
>> > net/irda/irnet/irnet_irda.c   irnet_dname_to_daddr()
>> > arch/powerpc/sysdev/ppc4xx_cpm.c static ssize_t cpm_idle_store()
>> > arch/x86/ras/mce_amd_inj.c static int __set_inj
>> > drivers/hwtracing/intel_th/msu.c mode_store
>> > drivers/pci/pcie/aer/ecrc.c void pcie_ecrc_get_policy
>> > drivers/pci/pcie/aspm.c pcie_aspm_set_policy
>> > drivers/scsi/aic7xxx/aic7xxx_osm.c aic7xxx_setup
>> > drivers/scsi/aic7xxx/aic79xx_osm.c aic79xx_setup
>> > drivers/scsi/scsi_transport_fc.c static int get_fc_##title##_match
>> > drivers/staging/android/ion/hisilicon/hi6220_ion.c get_type_by_name
>> > drivers/staging/lustre/lustre/lmv/lproc_lmv.c placement_name2policy
>> > drivers/xen/sys-hypervisor.c pmu_mode_store
>
> Thought more about those cases.
>
> If you would like you may introduce something like
>
> int nmatch_string(array, array_size, string, int len)
> {
>   if (len < 0)
>     return match_string();
>
>   for (...) {
>     size_t itemlen = (len > 0) ? len : strlen(array[index]);
> ...
>     if (!strncmp(array[index], string, itemlen))
>       return index;
>   }
>   return -EINVAL;
> }

Yeah, a separate function is probably better. But why not a more
explicit name, match_prefix, match_string_prefix, match_string_starts?

I like the idea of passing the string length if one wants the "is this a
prefix of some array element" semantics, and a sentinel otherwise. But I
don't see any case where one would want match_string() semantics (why
not call match_string directly instead?), so why not let len < 0 mean
"is some array element a prefix of this string" and "len >= 0" be the
other case. I don't see why one shouldn't be able to ask "is the empty
string a prefix of some array element" (that is, are there any elements
in the array); both the array and the string might be run-time things,
so this could occur. And it's not up to a generic library routine like
this to impose restrictions like "the empty string makes no sense, go
away".

Rasmus

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Andy Shevchenko]

On Tue, Jan 12, 2016 at 12:10 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On Mon, Jan 11 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
>> On Sat, 2016-01-09 at 13:57 +0200, Andy Shevchenko wrote:
>>> On Sat, Jan 9, 2016 at 3:12 AM, Sergey Senozhatsky
>>> <sergey.senozhatsky@gmail.com> wrote:
>>> > Andy Shevchenko wrote:
>>> > [..]
>>> > > >
>>> > > > strncmp() case seems to be quite common.
>>> > >
>>> > > Like I answered to Rasmus, please, provide real examples.
>>> >
>>> > [..]
>>> > > > int nmatch_string(array, array_size, string, string_len)
>>> > > > {
>>> > > >       do {
>>> > > >               strncmp();
>>> > > >       } while ();
>>> > > > }
>>> > > >
>>> > > > int match_string(array, array_size, string)
>>> > > > {
>>> > > >       return nmatch_string(array, array_size, string,
>>> > > > strlen(string));
>>> > > > }
>>> > >
>>> > > See above.
>>> >
>>> > after some quick and inaccurate grepping, well, probably you're
>>> > right - not worth it.
>>>
>>> Good grep anyway, it clearly shows that there is hard to generalize
>>> which limit to use: a) length of a first argument / item from a list,
>>> b) length of a second argument or a constant.
>>>
>>> > arch/mips/bcm63xx/boards/board_bcm963xx.c  void __init
>>> > board_prom_init(void)
>>> > net/irda/irnet/irnet_irda.c   irnet_dname_to_daddr()
>>> > arch/powerpc/sysdev/ppc4xx_cpm.c static ssize_t cpm_idle_store()
>>> > arch/x86/ras/mce_amd_inj.c static int __set_inj
>>> > drivers/hwtracing/intel_th/msu.c mode_store
>>> > drivers/pci/pcie/aer/ecrc.c void pcie_ecrc_get_policy
>>> > drivers/pci/pcie/aspm.c pcie_aspm_set_policy
>>> > drivers/scsi/aic7xxx/aic7xxx_osm.c aic7xxx_setup
>>> > drivers/scsi/aic7xxx/aic79xx_osm.c aic79xx_setup
>>> > drivers/scsi/scsi_transport_fc.c static int get_fc_##title##_match
>>> > drivers/staging/android/ion/hisilicon/hi6220_ion.c get_type_by_name
>>> > drivers/staging/lustre/lustre/lmv/lproc_lmv.c placement_name2policy
>>> > drivers/xen/sys-hypervisor.c pmu_mode_store
>>
>> Thought more about those cases.
>>
>> If you would like you may introduce something like
>>
>> int nmatch_string(array, array_size, string, int len)
>> {
>>   if (len < 0)
>>     return match_string();
>>
>>   for (...) {
>>     size_t itemlen = (len > 0) ? len : strlen(array[index]);
>> ...
>>     if (!strncmp(array[index], string, itemlen))
>>       return index;
>>   }
>>   return -EINVAL;
>> }
>
> Yeah, a separate function is probably better. But why not a more
> explicit name, match_prefix, match_string_prefix, match_string_starts?
>
> I like the idea of passing the string length if one wants the "is this a
> prefix of some array element" semantics, and a sentinel otherwise. But I
> don't see any case where one would want match_string() semantics (why
> not call match_string directly instead?), so why not let len < 0 mean
> "is some array element a prefix of this string" and "len >= 0" be the
> other case. I don't see why one shouldn't be able to ask "is the empty
> string a prefix of some array element" (that is, are there any elements
> in the array); both the array and the string might be run-time things,
> so this could occur. And it's not up to a generic library routine like
> this to impose restrictions like "the empty string makes no sense, go
> away".

I have no strong feelings to my initial proposal, and your suggestions
sound sane. Unfortunately I have no time to properly implement it and
convert users, so, if Sergey would like to do that, I will no object.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/8] lib/string: introduce match_string() helper

[Sergey Senozhatsky]

Hello,

On (01/11/16 23:10), Rasmus Villemoes wrote:
[..]
> > Thought more about those cases.
> >
> > If you would like you may introduce something like
> >
> > int nmatch_string(array, array_size, string, int len)
> > {
> >   if (len < 0)
> >     return match_string();
> >
> >   for (...) {
> >     size_t itemlen = (len > 0) ? len : strlen(array[index]);
> > ...
> >     if (!strncmp(array[index], string, itemlen))
> >       return index;
> >   }
> >   return -EINVAL;
> > }

may be later this week; I'm a bit out of spare time at the moment.


> Yeah, a separate function is probably better. But why not a more
> explicit name, match_prefix, match_string_prefix, match_string_starts?

Not married to nmatch_string(), but at the same time, IMHO, *_prefix or
*_starts naming is not really better. One can pass a string with offset,
e.g.
  FOO_starts(array, array_size, string + offset, strlen(string) - offset)
which will be equivalent to FOO_ends(), but not FOO_starts() or FOO_prefix().
Personally, I'd prefer to preserve strcmp/strncmp semantics, thus, forbidding
`len < 0' case, which looks cryptic to me.


> I like the idea of passing the string length if one wants the "is this a
> prefix of some array element" semantics, and a sentinel otherwise. But I
> don't see any case where one would want match_string() semantics (why
> not call match_string directly instead?),


> so why not let len < 0 mean "is some array element a prefix of this string"
> and "len >= 0" be the other case. I don't see why one shouldn't be able to
> ask "is the empty string a prefix of some array element" (that is, are there
> any elements in the array);

if this is a dynamic array, then there should be some function that
fills in that array, so having a simple bool flag in the code will
suffice; if this is a static array, then ARRAY_SIZE() should do the
trick. I would never expect a string matching function to have this
type of functionality, TBH.

But the question is

> is the empty string a prefix of some array element

do people really need this?

the way I see it, the idea is to have wrappers around
	while (array[..]) if strcmp()/strncmp() == 0 break ...

both of which [strcmp()/strncmp()] have a well known and expected
semantics, changing this can only confuse people.

	-ss

> both the array and the string might be run-time things,
> so this could occur. And it's not up to a generic library routine like
> this to impose restrictions like "the empty string makes no sense, go
> away".
> 
> Rasmus
>