Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325021 > unrolled thread

[PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name Chunyan Zhang <zhang.chunyan@linaro.org> - 2016-02-03 09:20 +0100
    [PATCH] stm class: fix semicolon.cocci warnings kbuild test robot <lkp@intel.com> - 2016-02-03 11:20 +0100
    Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid  STM device name kbuild test robot <lkp@intel.com> - 2016-02-03 11:20 +0100
    [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name Chunyan Zhang <zhang.chunyan@linaro.org> - 2016-02-04 10:00 +0100
      Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-02-04 18:40 +0100
        Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid  STM device name Chunyan Zhang <zhang.chunyan@linaro.org> - 2016-02-05 04:20 +0100

#1325021 — [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name

The node name of STM master management policy is a concatenation of an
STM device name to which this policy applies and following an arbitrary
string, these two strings are concatenated with a dot.

This patch adds a loop for extracting the STM device name when an
arbitrary number of dot(s) are found in this STM device name.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 11ab6d0..691686e 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
 	/*
 	 * node must look like <device_name>.<policy_name>, where
 	 * <device_name> is the name of an existing stm device and
-	 * <policy_name> is an arbitrary string
+	 * <policy_name> is an arbitrary string, when an arbitrary
+	 * number of dot(s) are found in the <device_name>, the
+	 * first matched STM device name would be extracted.
 	 */
-	p = strchr(devname, '.');
-	if (!p) {
-		kfree(devname);
-		return ERR_PTR(-EINVAL);
-	}
+	for (p = devname; ; p++) {
+		p = strchr(p, '.');
+		if (!p) {
+			kfree(devname);
+			return ERR_PTR(-EINVAL);
+		}
 
-	*p++ = '\0';
+		*p = '\0';
 
-	stm = stm_find_device(devname);
-	kfree(devname);
+		stm = stm_find_device(devname);
+		if (stm)
+			break;
+		*p = '.';
+	};
 
-	if (!stm)
-		return ERR_PTR(-ENODEV);
+	kfree(devname);
 
 	mutex_lock(&stm->policy_mutex);
 	if (stm->policy) {
-- 
1.9.1

[toc] | [next] | [standalone]

#1325128 — [PATCH] stm class: fix semicolon.cocci warnings

drivers/hwtracing/stm/policy.c:341:2-3: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

CC: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 policy.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -338,7 +338,7 @@ stp_policies_make(struct config_group *g
 		if (stm)
 			break;
 		*p = '.';
-	};
+	}
 
 	kfree(devname);
 

[toc] | [prev] | [next] | [standalone]

#1325131 — Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name

Hi Chunyan,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.5-rc2 next-20160203]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Chunyan-Zhang/Introduce-CoreSight-STM-support/20160203-161836
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/hwtracing/stm/policy.c:341:2-3: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1326495

The node name of STM master management policy is a concatenation of an
STM device name to which this policy applies and following an arbitrary
string, these two strings are concatenated with a dot.

This patch adds a loop for extracting the STM device name when an
arbitrary number of dot(s) are found in this STM device name.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 11ab6d0..691686e 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
 	/*
 	 * node must look like <device_name>.<policy_name>, where
 	 * <device_name> is the name of an existing stm device and
-	 * <policy_name> is an arbitrary string
+	 * <policy_name> is an arbitrary string, when an arbitrary
+	 * number of dot(s) are found in the <device_name>, the
+	 * first matched STM device name would be extracted.
 	 */
-	p = strchr(devname, '.');
-	if (!p) {
-		kfree(devname);
-		return ERR_PTR(-EINVAL);
-	}
+	for (p = devname; ; p++) {
+		p = strchr(p, '.');
+		if (!p) {
+			kfree(devname);
+			return ERR_PTR(-EINVAL);
+		}
 
-	*p++ = '\0';
+		*p = '\0';
 
-	stm = stm_find_device(devname);
-	kfree(devname);
+		stm = stm_find_device(devname);
+		if (stm)
+			break;
+		*p = '.';
+	}
 
-	if (!stm)
-		return ERR_PTR(-ENODEV);
+	kfree(devname);
 
 	mutex_lock(&stm->policy_mutex);
 	if (stm->policy) {
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1327055

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

I few comments below:

> The node name of STM master management policy is a concatenation of an
> STM device name to which this policy applies and following an arbitrary
> string, these two strings are concatenated with a dot.

This is true.

> This patch adds a loop for extracting the STM device name when an
> arbitrary number of dot(s) are found in this STM device name.

It's not very easy to tell what's going on here from this
description. The reader be left curious as to why an arbitrary number of
dots is a reason to run a loop. When in doubt, try to imagine as if
you're seeing this patch for the first time and ask yourself, does the
message give a clear explanation of what's going on in it.

> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index 11ab6d0..691686e 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
>  	/*
>  	 * node must look like <device_name>.<policy_name>, where
>  	 * <device_name> is the name of an existing stm device and
> -	 * <policy_name> is an arbitrary string
> +	 * <policy_name> is an arbitrary string, when an arbitrary
> +	 * number of dot(s) are found in the <device_name>, the
> +	 * first matched STM device name would be extracted.
>  	 */

This leaves room for a number of suspicious situations. What if both
"xyz" and "xyz.0" are stm devices, how would you create a policy for the
latter, for example?

The rules should be such that you can tell exactly what the intended stm
device is from a policy directory name, otherwise it's just asking for
trouble.

> -	p = strchr(devname, '.');
> -	if (!p) {
> -		kfree(devname);
> -		return ERR_PTR(-EINVAL);
> -	}
> +	for (p = devname; ; p++) {
> +		p = strchr(p, '.');
> +		if (!p) {
> +			kfree(devname);
> +			return ERR_PTR(-EINVAL);
> +		}
>  
> -	*p++ = '\0';
> +		*p = '\0';
>  
> -	stm = stm_find_device(devname);
> -	kfree(devname);
> +		stm = stm_find_device(devname);
> +		if (stm)
> +			break;
> +		*p = '.';
> +	}
>  
> -	if (!stm)
> -		return ERR_PTR(-ENODEV);
> +	kfree(devname);

In the existing code there is a clear distinction between -ENODEV, which
is to say "we didn't find the device" and -EINVAL, "directory name
breaks rules/is badly formatted". After the change, it's all -EINVAL,
which also becomes "we tried everything, sorry".

So, having said all that, does the following patch solve your problem:

From 870dc5fefa5623c39552511d31e0fa0da984d581 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Thu, 4 Feb 2016 18:56:34 +0200
Subject: [PATCH] stm class: Support devices with multiple instances

By convention, the name of the stm policy directory in configfs consists of
the device name to which it applies and the actual policy name, separated
by a dot. Now, some devices already have dots in their names that separate
name of the actual device from its instance identifier. Such devices will
result in two (or more, who can tell) dots in the policy directory name.

Existing policy code, however, will treat the first dot as the one that
separates device name from policy name, therefore failing the above case.

This patch makes the last dot in the directory name be the separator, thus
prohibiting dots from being used in policy names.

Suggested-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/policy.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 94d3abfb73..1db189657b 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -332,10 +332,11 @@ stp_policies_make(struct config_group *group, const char *name)
 
 	/*
 	 * node must look like <device_name>.<policy_name>, where
-	 * <device_name> is the name of an existing stm device and
-	 * <policy_name> is an arbitrary string
+	 * <device_name> is the name of an existing stm device; may
+	 *               contain dots;
+	 * <policy_name> is an arbitrary string; may not contain dots
 	 */
-	p = strchr(devname, '.');
+	p = strrchr(devname, '.');
 	if (!p) {
 		kfree(devname);
 		return ERR_PTR(-EINVAL);
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1327454 — Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name

On Fri, Feb 5, 2016 at 1:30 AM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
> I few comments below:
>
>> The node name of STM master management policy is a concatenation of an
>> STM device name to which this policy applies and following an arbitrary
>> string, these two strings are concatenated with a dot.
>
> This is true.
>
>> This patch adds a loop for extracting the STM device name when an
>> arbitrary number of dot(s) are found in this STM device name.
>
> It's not very easy to tell what's going on here from this
> description. The reader be left curious as to why an arbitrary number of
> dots is a reason to run a loop. When in doubt, try to imagine as if
> you're seeing this patch for the first time and ask yourself, does the
> message give a clear explanation of what's going on in it.
>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> ---
>>  drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
>> index 11ab6d0..691686e 100644
>> --- a/drivers/hwtracing/stm/policy.c
>> +++ b/drivers/hwtracing/stm/policy.c
>> @@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
>>       /*
>>        * node must look like <device_name>.<policy_name>, where
>>        * <device_name> is the name of an existing stm device and
>> -      * <policy_name> is an arbitrary string
>> +      * <policy_name> is an arbitrary string, when an arbitrary
>> +      * number of dot(s) are found in the <device_name>, the
>> +      * first matched STM device name would be extracted.
>>        */
>
> This leaves room for a number of suspicious situations. What if both
> "xyz" and "xyz.0" are stm devices, how would you create a policy for the
> latter, for example?
>
> The rules should be such that you can tell exactly what the intended stm
> device is from a policy directory name, otherwise it's just asking for
> trouble.
>
>> -     p = strchr(devname, '.');
>> -     if (!p) {
>> -             kfree(devname);
>> -             return ERR_PTR(-EINVAL);
>> -     }
>> +     for (p = devname; ; p++) {
>> +             p = strchr(p, '.');
>> +             if (!p) {
>> +                     kfree(devname);
>> +                     return ERR_PTR(-EINVAL);
>> +             }
>>
>> -     *p++ = '\0';
>> +             *p = '\0';
>>
>> -     stm = stm_find_device(devname);
>> -     kfree(devname);
>> +             stm = stm_find_device(devname);
>> +             if (stm)
>> +                     break;
>> +             *p = '.';
>> +     }
>>
>> -     if (!stm)
>> -             return ERR_PTR(-ENODEV);
>> +     kfree(devname);
>
> In the existing code there is a clear distinction between -ENODEV, which
> is to say "we didn't find the device" and -EINVAL, "directory name
> breaks rules/is badly formatted". After the change, it's all -EINVAL,
> which also becomes "we tried everything, sorry".
>
> So, having said all that, does the following patch solve your problem:

Yes, I originally modified as well like your following patch, but at
that moment, I didn't get your agreement that the policy name (i.e. an
arbitrary string) cannot contain dots, so I had to consider the case.
Whatever, there isn't a panacea.  I'm very good with your patch.  Many
thanks for your review and providing the patch.


Chunyan

>
> From 870dc5fefa5623c39552511d31e0fa0da984d581 Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Thu, 4 Feb 2016 18:56:34 +0200
> Subject: [PATCH] stm class: Support devices with multiple instances
>
> By convention, the name of the stm policy directory in configfs consists of
> the device name to which it applies and the actual policy name, separated
> by a dot. Now, some devices already have dots in their names that separate
> name of the actual device from its instance identifier. Such devices will
> result in two (or more, who can tell) dots in the policy directory name.
>
> Existing policy code, however, will treat the first dot as the one that
> separates device name from policy name, therefore failing the above case.
>
> This patch makes the last dot in the directory name be the separator, thus
> prohibiting dots from being used in policy names.
>
> Suggested-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/stm/policy.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index 94d3abfb73..1db189657b 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -332,10 +332,11 @@ stp_policies_make(struct config_group *group, const char *name)
>
>         /*
>          * node must look like <device_name>.<policy_name>, where
> -        * <device_name> is the name of an existing stm device and
> -        * <policy_name> is an arbitrary string
> +        * <device_name> is the name of an existing stm device; may
> +        *               contain dots;
> +        * <policy_name> is an arbitrary string; may not contain dots
>          */
> -       p = strchr(devname, '.');
> +       p = strrchr(devname, '.');
>         if (!p) {
>                 kfree(devname);
>                 return ERR_PTR(-EINVAL);
> --
> 2.7.0
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web