Re: [PATCH 1/5] checks: Add Warning for stricter property name character checking

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

On Tue, Jan 24, 2017 at 11:45:30AM -0600, Rob Herring wrote:
> While '?', '.', '+', '*', and '_' are considered valid characters their
> use is discouraged in recommended practices. '#' is also only
> recommended to be used at the beginning of property names.

Hrm.  I'm not totally convinced about this.  PAPR definitely specifies
a few properties that include '#' not at the beginning.  I'm not sure
that IEEE1275 doesn't as well (it's not exactly easy to grep for a
single character).

I'm not sure about '?' either.  I didn't positively identify any
properties including it, but there are certainly OF configuration
variables specified by PAPR including '?', and I think those will be
exposed in the device tree as properties.

I'm not 100% sure about '*' either, I have a vague memory of some spec
that requires it, but I'm not certain.

AFAIK, excluding '.', '+' and '_' should be ok, except for device_type
which you already have special exception for.

> Testing this found one typo error with '.' used instead of ','. The
> rest of the warnings were all from underscores.
> 
> Signed-off-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  checks.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index 3d18e45374c8..a0d4a9d968d7 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -239,6 +239,7 @@ ERROR(duplicate_property_names, check_duplicate_property_names, NULL);
>  #define UPPERCASE	"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
>  #define DIGITS		"0123456789"
>  #define PROPNODECHARS	LOWERCASE UPPERCASE DIGITS ",._+*#?-"
> +#define PROPNODECHARSSTRICT	LOWERCASE UPPERCASE DIGITS ",-"
>  
>  static void check_node_name_chars(struct check *c, struct dt_info *dti,
>  				  struct node *node)
> @@ -299,6 +300,38 @@ static void check_property_name_chars(struct check *c, struct dt_info *dti,
>  }
>  ERROR(property_name_chars, check_property_name_chars, PROPNODECHARS);
>  
> +static void check_property_name_chars_strict(struct check *c,
> +					     struct dt_info *dti,
> +					     struct node *node)
> +{
> +	struct property *prop;
> +
> +	for_each_property(node, prop) {
> +		const char *name = prop->name;
> +		int n = strspn(name, c->data);
> +
> +		if (n == strlen(prop->name))
> +			continue;
> +
> +		/* Certain names are whitelisted */
> +		if (strcmp(name, "device_type") == 0)
> +			continue;
> +
> +		/*
> +		 * # is only allowed at the beginning of property names not counting
> +		 * the vendor prefix.
> +		 */
> +		if (name[n] == '#' && ((n == 0) || (name[n-1] == ','))) {
> +			name += n + 1;
> +			n = strspn(name, c->data);
> +		}
> +		if (n < strlen(name))
> +			FAIL(c, "Character '%c' not recommended in property name \"%s\", node %s",
> +			     name[n], prop->name, node->fullpath);
> +	}
> +}
> +WARNING(property_name_chars_strict, check_property_name_chars_strict, PROPNODECHARSSTRICT);
> +
>  #define DESCLABEL_FMT	"%s%s%s%s%s"
>  #define DESCLABEL_ARGS(node,prop,mark)		\
>  	((mark) ? "value of " : ""),		\
> @@ -703,6 +736,8 @@ static struct check *check_table[] = {
>  	&address_cells_is_cell, &size_cells_is_cell, &interrupt_cells_is_cell,
>  	&device_type_is_string, &model_is_string, &status_is_string,
>  
> +	&property_name_chars_strict,
> +
>  	&addr_size_cells, &reg_format, &ranges_format,
>  
>  	&unit_address_vs_reg,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]