[v5,05/14] software node: clean up property_copy_string_array()
diff mbox series

Message ID 20191011230721.206646-6-dmitry.torokhov@gmail.com
State Superseded, archived
Headers show
Series
  • software node: add support for reference properties
Related show

Commit Message

Dmitry Torokhov Oct. 11, 2019, 11:07 p.m. UTC
Because property_copy_string_array() stores the newly allocated pointer in the
destination property, we have an awkward code in property_entry_copy_data()
where we fetch the new pointer from dst.

Let's change property_copy_string_array() to return pointer and rely on the
common path in property_entry_copy_data() to store it in destination structure.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

Comments

Andy Shevchenko Oct. 15, 2019, 12:07 p.m. UTC | #1
On Fri, Oct 11, 2019 at 04:07:12PM -0700, Dmitry Torokhov wrote:
> Because property_copy_string_array() stores the newly allocated pointer in the
> destination property, we have an awkward code in property_entry_copy_data()
> where we fetch the new pointer from dst.

I don't see a problem in this function.

Rather 'awkward code' is a result of use property_set_pointer() which relies on
data type.

> Let's change property_copy_string_array() to return pointer and rely on the
> common path in property_entry_copy_data() to store it in destination structure.
Dmitry Torokhov Oct. 15, 2019, 6:12 p.m. UTC | #2
On Tue, Oct 15, 2019 at 03:07:26PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:12PM -0700, Dmitry Torokhov wrote:
> > Because property_copy_string_array() stores the newly allocated pointer in the
> > destination property, we have an awkward code in property_entry_copy_data()
> > where we fetch the new pointer from dst.
> 
> I don't see a problem in this function.
> 
> Rather 'awkward code' is a result of use property_set_pointer() which relies on
> data type.

No, the awkwardness is that we set the pointer once in
property_copy_string_array(), then fetch it in
property_entry_copy_data() only to set it again via
property_set_pointer(). This is confising and awkward and I believe it
is cleaner for property_copy_string_array() to give a pointer to a copy
of a string array, and then property_entry_copy_data() use it when
handling the destination structure.

Thanks.
Andy Shevchenko Oct. 16, 2019, 7:53 a.m. UTC | #3
On Tue, Oct 15, 2019 at 11:12:11AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 15, 2019 at 03:07:26PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 11, 2019 at 04:07:12PM -0700, Dmitry Torokhov wrote:
> > > Because property_copy_string_array() stores the newly allocated pointer in the
> > > destination property, we have an awkward code in property_entry_copy_data()
> > > where we fetch the new pointer from dst.
> > 
> > I don't see a problem in this function.
> > 
> > Rather 'awkward code' is a result of use property_set_pointer() which relies on
> > data type.
> 
> No, the awkwardness is that we set the pointer once in
> property_copy_string_array(), then fetch it in
> property_entry_copy_data() only to set it again via
> property_set_pointer().

Yes, since property_set_pointer is called independently
on the type of the value.


> This is confising and awkward and I believe it
> is cleaner for property_copy_string_array() to give a pointer to a copy
> of a string array, and then property_entry_copy_data() use it when
> handling the destination structure.

We probably need a 3rd opinion here.
Dmitry Torokhov Oct. 16, 2019, 5 p.m. UTC | #4
On Wed, Oct 16, 2019 at 10:53:00AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 15, 2019 at 11:12:11AM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 15, 2019 at 03:07:26PM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 11, 2019 at 04:07:12PM -0700, Dmitry Torokhov wrote:
> > > > Because property_copy_string_array() stores the newly allocated pointer in the
> > > > destination property, we have an awkward code in property_entry_copy_data()
> > > > where we fetch the new pointer from dst.
> > > 
> > > I don't see a problem in this function.
> > > 
> > > Rather 'awkward code' is a result of use property_set_pointer() which relies on
> > > data type.
> > 
> > No, the awkwardness is that we set the pointer once in
> > property_copy_string_array(), then fetch it in
> > property_entry_copy_data() only to set it again via
> > property_set_pointer().
> 
> Yes, since property_set_pointer is called independently
> on the type of the value.

We still call property_set_pointer() independently of the type of the
value even with this patch. The point is that we do not set the pointer
in property_copy_string_array(), so we only set the pointer once.

We used to have essentially for string arrays:

	copy data
	set pointer in dst
	get pointer from dst
	set pointer in dst

With this patch we have:

	copy data
	set pointer in dst

> 
> 
> > This is confising and awkward and I believe it
> > is cleaner for property_copy_string_array() to give a pointer to a copy
> > of a string array, and then property_entry_copy_data() use it when
> > handling the destination structure.
> 
> We probably need a 3rd opinion here.

I think I can still convince you ;)

Thanks.
Andy Shevchenko Oct. 17, 2019, 7:02 a.m. UTC | #5
On Wed, Oct 16, 2019 at 10:00:59AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 16, 2019 at 10:53:00AM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 15, 2019 at 11:12:11AM -0700, Dmitry Torokhov wrote:
> > > On Tue, Oct 15, 2019 at 03:07:26PM +0300, Andy Shevchenko wrote:

> > Yes, since property_set_pointer is called independently
> > on the type of the value.
> 
> We still call property_set_pointer() independently of the type of the
> value even with this patch. The point is that we do not set the pointer
> in property_copy_string_array(), so we only set the pointer once.
> 
> We used to have essentially for string arrays:
> 
> 	copy data
> 	set pointer in dst
> 	get pointer from dst
> 	set pointer in dst
> 
> With this patch we have:
> 
> 	copy data
> 	set pointer in dst

> > > This is confising and awkward and I believe it
> > > is cleaner for property_copy_string_array() to give a pointer to a copy
> > > of a string array, and then property_entry_copy_data() use it when
> > > handling the destination structure.
> > 
> > We probably need a 3rd opinion here.
> 
> I think I can still convince you ;)

Probably this is fine.

Patch
diff mbox series

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index a1f3f0994f9f..2f2248c9003c 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -337,8 +337,8 @@  static void property_entry_free_data(const struct property_entry *p)
 	kfree(p->name);
 }
 
-static int property_copy_string_array(struct property_entry *dst,
-				      const struct property_entry *src)
+static const char * const *
+property_copy_string_array(const struct property_entry *src)
 {
 	const char **d;
 	size_t nval = src->length / sizeof(*d);
@@ -346,7 +346,7 @@  static int property_copy_string_array(struct property_entry *dst,
 
 	d = kcalloc(nval, sizeof(*d), GFP_KERNEL);
 	if (!d)
-		return -ENOMEM;
+		return NULL;
 
 	for (i = 0; i < nval; i++) {
 		d[i] = kstrdup(src->pointer.str[i], GFP_KERNEL);
@@ -354,12 +354,11 @@  static int property_copy_string_array(struct property_entry *dst,
 			while (--i >= 0)
 				kfree(d[i]);
 			kfree(d);
-			return -ENOMEM;
+			return NULL;
 		}
 	}
 
-	dst->pointer.str = d;
-	return 0;
+	return d;
 }
 
 static int property_entry_copy_data(struct property_entry *dst,
@@ -367,17 +366,15 @@  static int property_entry_copy_data(struct property_entry *dst,
 {
 	const void *pointer = property_get_pointer(src);
 	const void *new;
-	int error;
 
 	if (src->is_array) {
 		if (!src->length)
 			return -ENODATA;
 
 		if (src->type == DEV_PROP_STRING) {
-			error = property_copy_string_array(dst, src);
-			if (error)
-				return error;
-			new = dst->pointer.str;
+			new = property_copy_string_array(src);
+			if (!new)
+				return -ENOMEM;
 		} else {
 			new = kmemdup(pointer, src->length, GFP_KERNEL);
 			if (!new)