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

Groups > linux.kernel > #1572842 > unrolled thread

[PATCH v5 2/4] device property: constify property arrays values

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 v5 2/4] device property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-03 02:50 +0100
    Re: [PATCH v5 2/4] device property: constify property arrays values Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-02-03 12:50 +0100
      Re: [PATCH v5 2/4] device property: constify property arrays values Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-03 16:20 +0100

#1572842 — [PATCH v5 2/4] device property: constify property arrays values

Data that is fed into property arrays should not be modified, so let's mark
relevant pointers as const. This will allow us making source arrays as
const/__initconst.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/property.c  | 10 +++++-----
 include/linux/property.h | 12 ++++++------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e9fa75645d69..31b942a29fdc 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -718,7 +718,8 @@ static void pset_free_set(struct property_set *pset)
 static int pset_copy_entry(struct property_entry *dst,
 			   const struct property_entry *src)
 {
-	const char **d, **s;
+	const char * const *s;
+	char **d;
 	size_t i, nval;
 
 	dst->name = kstrdup(src->name, GFP_KERNEL);
@@ -731,12 +732,11 @@ static int pset_copy_entry(struct property_entry *dst,
 
 		if (src->is_string) {
 			nval = src->length / sizeof(const char *);
-			dst->pointer.str = kcalloc(nval, sizeof(const char *),
-						   GFP_KERNEL);
-			if (!dst->pointer.str)
+			d = kcalloc(nval, sizeof(const char *), GFP_KERNEL);
+			if (!d)
 				return -ENOMEM;
 
-			d = dst->pointer.str;
+			dst->pointer.raw_data = d;
 			s = src->pointer.str;
 			for (i = 0; i < nval; i++) {
 				d[i] = kstrdup(s[i], GFP_KERNEL);
diff --git a/include/linux/property.h b/include/linux/property.h
index d37a4498b3ac..7a0a1cce5165 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -160,12 +160,12 @@ struct property_entry {
 	bool is_string;
 	union {
 		union {
-			void *raw_data;
-			u8 *u8_data;
-			u16 *u16_data;
-			u32 *u32_data;
-			u64 *u64_data;
-			const char **str;
+			const void *raw_data;
+			const u8 *u8_data;
+			const u16 *u16_data;
+			const u32 *u32_data;
+			const u64 *u64_data;
+			const char * const *str;
 		} pointer;
 		union {
 			unsigned long long raw_data;
-- 
2.11.0.483.g087da7b7c-goog

[toc] | [next] | [standalone]

#1573040

On Thu, 2017-02-02 at 17:41 -0800, Dmitry Torokhov wrote:
> Data that is fed into property arrays should not be modified, so let's
> mark
> relevant pointers as const. This will allow us making source arrays as
> const/__initconst.
> 

> @@ -718,7 +718,8 @@ static void pset_free_set(struct property_set
> *pset)
>  static int pset_copy_entry(struct property_entry *dst,
>  			   const struct property_entry *src)
>  {
> -	const char **d, **s;
> +	const char * const *s;
> +	char **d;

You removed const here

>  	size_t i, nval;
>  
>  	dst->name = kstrdup(src->name, GFP_KERNEL);
> @@ -731,12 +732,11 @@ static int pset_copy_entry(struct property_entry
> *dst,
>  
>  		if (src->is_string) {
>  			nval = src->length / sizeof(const char *);
> -			dst->pointer.str = kcalloc(nval, sizeof(const
> char *),
> -						   GFP_KERNEL);
> -			if (!dst->pointer.str)
> +			d = kcalloc(nval, sizeof(const char *),
> GFP_KERNEL);

But left it here. Do we need to remove const?

> +			if (!d)
>  				return -ENOMEM;
>  
> -			d = dst->pointer.str;
> +			dst->pointer.raw_data = d;
>  			s = src->pointer.str;

So, overall, do we need these changes at all? Nothing in commit message
sheds a light on it.

>  			for (i = 0; i < nval; i++) {
>  				d[i] = kstrdup(s[i], GFP_KERNEL);

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

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

#1573143

On Fri, Feb 03, 2017 at 01:43:03PM +0200, Andy Shevchenko wrote:
> On Thu, 2017-02-02 at 17:41 -0800, Dmitry Torokhov wrote:
> > Data that is fed into property arrays should not be modified, so let's
> > mark
> > relevant pointers as const. This will allow us making source arrays as
> > const/__initconst.
> > 
> 
> > @@ -718,7 +718,8 @@ static void pset_free_set(struct property_set
> > *pset)
> >  static int pset_copy_entry(struct property_entry *dst,
> >  			   const struct property_entry *src)
> >  {
> > -	const char **d, **s;
> > +	const char * const *s;
> > +	char **d;
> 
> You removed const here

Yes I did. It is hard to assign value to a constant otherwise.

> 
> >  	size_t i, nval;
> >  
> >  	dst->name = kstrdup(src->name, GFP_KERNEL);
> > @@ -731,12 +732,11 @@ static int pset_copy_entry(struct property_entry
> > *dst,
> >  
> >  		if (src->is_string) {
> >  			nval = src->length / sizeof(const char *);
> > -			dst->pointer.str = kcalloc(nval, sizeof(const
> > char *),
> > -						   GFP_KERNEL);
> > -			if (!dst->pointer.str)
> > +			d = kcalloc(nval, sizeof(const char *),
> > GFP_KERNEL);
> 
> But left it here. Do we need to remove const?

I do not know why we had it in the first place: the size is the samei
between constant and variable of the same type.

Ideally we'd use sizeof(*d), I can do it after this batch is accepted.

> 
> > +			if (!d)
> >  				return -ENOMEM;
> >  
> > -			d = dst->pointer.str;
> > +			dst->pointer.raw_data = d;
> >  			s = src->pointer.str;
> 
> So, overall, do we need these changes at all? Nothing in commit message
> sheds a light on it.

The compiler insists in them though.

> 
> >  			for (i = 0; i < nval; i++) {
> >  				d[i] = kstrdup(s[i], GFP_KERNEL);

Thanks.

-- 
Dmitry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web