| Message ID | 20191011230721.206646-6-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | software node: add support for reference properties | expand |
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.
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.
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.
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.
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.
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)
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(-)