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

[Alexander Shishkin]

Chunyan Zhang <zhang.chunyan@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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: