| Message ID | 20200928104155.7385-2-brgl@bgdev.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: mockup: refactoring + documentation | expand |
On Mon, Sep 28, 2020 at 12:41:47PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > There's a common pattern of dynamically allocating an array of char > pointers and then also dynamically allocating each string in this > array. Provide a helper for freeing such a string array with one call. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> But see below. > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > include/linux/string_helpers.h | 2 ++ > lib/string_helpers.c | 25 +++++++++++++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h > index 86f150c2a6b6..55b25120a1c6 100644 > --- a/include/linux/string_helpers.h > +++ b/include/linux/string_helpers.h > @@ -94,4 +94,6 @@ char *kstrdup_quotable(const char *src, gfp_t gfp); > char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp); > char *kstrdup_quotable_file(struct file *file, gfp_t gfp); > > +void kfree_strarray(char **str_array, size_t num_str); > + > #endif > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index 963050c0283e..bfa4c9f3ca0a 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -649,3 +649,28 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp) > return pathname; > } > EXPORT_SYMBOL_GPL(kstrdup_quotable_file); > + > +/** > + * kfree_strarray - free a number of dynamically allocated strings contained > + * in an array and the array itself > + * > + * @str_array: Dynamically allocated array of strings to free. If NULL - the > + * function does nothing. > + * @num_str: Number of strings (starting from the beginning of the array) to > + * free. > + * > + * Passing a non-null str_array and num_str == 0 as well as NULL str_array > + * are valid use-cases. > + */ > +void kfree_strarray(char **str_array, size_t num_str) Hmm... I have missed your answer to str_array -> array num_str -> n The rationale behind dropping str is to avoid duplicates in the name of the function and its parameters. 'array' is harder to avoid, but also possible, though I leave it to you. > +{ > + unsigned int i; > + > + if (!str_array) > + return; > + > + for (i = 0; i < num_str; i++) > + kfree(str_array[i]); > + kfree(str_array); > +} > +EXPORT_SYMBOL_GPL(kfree_strarray); > -- > 2.26.1 > -- With Best Regards, Andy Shevchenko
On Mon, Sep 28, 2020 at 2:55 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 28, 2020 at 12:41:47PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > There's a common pattern of dynamically allocating an array of char > > pointers and then also dynamically allocating each string in this > > array. Provide a helper for freeing such a string array with one call. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > But see below. > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > include/linux/string_helpers.h | 2 ++ > > lib/string_helpers.c | 25 +++++++++++++++++++++++++ > > 2 files changed, 27 insertions(+) > > > > diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h > > index 86f150c2a6b6..55b25120a1c6 100644 > > --- a/include/linux/string_helpers.h > > +++ b/include/linux/string_helpers.h > > @@ -94,4 +94,6 @@ char *kstrdup_quotable(const char *src, gfp_t gfp); > > char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp); > > char *kstrdup_quotable_file(struct file *file, gfp_t gfp); > > > > +void kfree_strarray(char **str_array, size_t num_str); > > + > > #endif > > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > > index 963050c0283e..bfa4c9f3ca0a 100644 > > --- a/lib/string_helpers.c > > +++ b/lib/string_helpers.c > > @@ -649,3 +649,28 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp) > > return pathname; > > } > > EXPORT_SYMBOL_GPL(kstrdup_quotable_file); > > + > > +/** > > + * kfree_strarray - free a number of dynamically allocated strings contained > > + * in an array and the array itself > > + * > > + * @str_array: Dynamically allocated array of strings to free. If NULL - the > > + * function does nothing. > > + * @num_str: Number of strings (starting from the beginning of the array) to > > + * free. > > + * > > + * Passing a non-null str_array and num_str == 0 as well as NULL str_array > > + * are valid use-cases. > > + */ > > +void kfree_strarray(char **str_array, size_t num_str) > > Hmm... I have missed your answer to > str_array -> array > num_str -> n > > The rationale behind dropping str is to avoid duplicates in the name of the > function and its parameters. 'array' is harder to avoid, but also possible, > though I leave it to you. > Are you fine with me fixing this when applying the patches? Bart
On Mon, Sep 28, 2020 at 03:04:05PM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 28, 2020 at 2:55 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Sep 28, 2020 at 12:41:47PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > There's a common pattern of dynamically allocating an array of char > > > pointers and then also dynamically allocating each string in this > > > array. Provide a helper for freeing such a string array with one call. > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > But see below. > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > --- > > > include/linux/string_helpers.h | 2 ++ > > > lib/string_helpers.c | 25 +++++++++++++++++++++++++ > > > 2 files changed, 27 insertions(+) > > > > > > diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h > > > index 86f150c2a6b6..55b25120a1c6 100644 > > > --- a/include/linux/string_helpers.h > > > +++ b/include/linux/string_helpers.h > > > @@ -94,4 +94,6 @@ char *kstrdup_quotable(const char *src, gfp_t gfp); > > > char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp); > > > char *kstrdup_quotable_file(struct file *file, gfp_t gfp); > > > > > > +void kfree_strarray(char **str_array, size_t num_str); > > > + > > > #endif > > > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > > > index 963050c0283e..bfa4c9f3ca0a 100644 > > > --- a/lib/string_helpers.c > > > +++ b/lib/string_helpers.c > > > @@ -649,3 +649,28 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp) > > > return pathname; > > > } > > > EXPORT_SYMBOL_GPL(kstrdup_quotable_file); > > > + > > > +/** > > > + * kfree_strarray - free a number of dynamically allocated strings contained > > > + * in an array and the array itself > > > + * > > > + * @str_array: Dynamically allocated array of strings to free. If NULL - the > > > + * function does nothing. > > > + * @num_str: Number of strings (starting from the beginning of the array) to > > > + * free. > > > + * > > > + * Passing a non-null str_array and num_str == 0 as well as NULL str_array > > > + * are valid use-cases. > > > + */ > > > +void kfree_strarray(char **str_array, size_t num_str) > > > > Hmm... I have missed your answer to > > str_array -> array > > num_str -> n > > > > The rationale behind dropping str is to avoid duplicates in the name of the > > function and its parameters. 'array' is harder to avoid, but also possible, > > though I leave it to you. > > > > Are you fine with me fixing this when applying the patches? Sure! -- With Best Regards, Andy Shevchenko
On Mon, 2020-09-28 at 12:41 +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > There's a common pattern of dynamically allocating an array of char > pointers and then also dynamically allocating each string in this > array. Provide a helper for freeing such a string array with one call. Isn't this also common for things like ring buffers? Why limit this to char *[]?
On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote: > > On Mon, 2020-09-28 at 12:41 +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > There's a common pattern of dynamically allocating an array of char > > pointers and then also dynamically allocating each string in this > > array. Provide a helper for freeing such a string array with one call. > > Isn't this also common for things like ring buffers? > Why limit this to char *[]? > I don't want to add APIs nobody is using. What do you suggest? Bartosz
On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote: > On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote: > > On Mon, 2020-09-28 at 12:41 +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > There's a common pattern of dynamically allocating an array of char > > > pointers and then also dynamically allocating each string in this > > > array. Provide a helper for freeing such a string array with one call. > > > > Isn't this also common for things like ring buffers? > > Why limit this to char *[]? > > > > I don't want to add APIs nobody is using. What do you suggest? Change the argument to void** and call it void kfree_array(void **array, int count);
On Mon, Sep 28, 2020 at 09:06:34AM -0700, Joe Perches wrote: > On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote: > > > On Mon, 2020-09-28 at 12:41 +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > There's a common pattern of dynamically allocating an array of char > > > > pointers and then also dynamically allocating each string in this > > > > array. Provide a helper for freeing such a string array with one call. > > > > > > Isn't this also common for things like ring buffers? > > > Why limit this to char *[]? > > > > > > > I don't want to add APIs nobody is using. What do you suggest? > > Change the argument to void** and call it > > void kfree_array(void **array, int count); Bart, if you go for this, I'm fine. You may keep my tag.
From: Joe Perches > Sent: 28 September 2020 17:07 > > On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote: > > > On Mon, 2020-09-28 at 12:41 +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > There's a common pattern of dynamically allocating an array of char > > > > pointers and then also dynamically allocating each string in this > > > > array. Provide a helper for freeing such a string array with one call. > > > > > > Isn't this also common for things like ring buffers? > > > Why limit this to char *[]? > > > > > > > I don't want to add APIs nobody is using. What do you suggest? > > Change the argument to void** and call it > > void kfree_array(void **array, int count); Does help, void doesn't work that way. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Sep 29, 2020 at 08:10:10AM +0000, David Laight wrote: > From: Joe Perches > > Sent: 28 September 2020 17:07 > > > > On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote: > > > > On Mon, 2020-09-28 at 12:41 +0200, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > > > There's a common pattern of dynamically allocating an array of char > > > > > pointers and then also dynamically allocating each string in this > > > > > array. Provide a helper for freeing such a string array with one call. > > > > > > > > Isn't this also common for things like ring buffers? > > > > Why limit this to char *[]? > > > > > > > > > > I don't want to add APIs nobody is using. What do you suggest? > > > > Change the argument to void** and call it > > > > void kfree_array(void **array, int count); > > Does help, void doesn't work that way. Actually good catch. void * and void ** have a big difference in the implicit casting behaviour. I was stumbled over this while playing with some experimental stuff locally.
On Tue, Sep 29, 2020 at 10:49 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Sep 29, 2020 at 08:10:10AM +0000, David Laight wrote: > > From: Joe Perches > > > Sent: 28 September 2020 17:07 > > > > > > On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote: > > > > On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote: > > > > > On Mon, 2020-09-28 at 12:41 +0200, Bartosz Golaszewski wrote: > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > > > > > There's a common pattern of dynamically allocating an array of char > > > > > > pointers and then also dynamically allocating each string in this > > > > > > array. Provide a helper for freeing such a string array with one call. > > > > > > > > > > Isn't this also common for things like ring buffers? > > > > > Why limit this to char *[]? > > > > > > > > > > > > > I don't want to add APIs nobody is using. What do you suggest? > > > > > > Change the argument to void** and call it > > > > > > void kfree_array(void **array, int count); > > > > Does help, void doesn't work that way. > > Actually good catch. void * and void ** have a big difference in the implicit > casting behaviour. I was stumbled over this while playing with some > experimental stuff locally. > I'll keep kfree_strarray() then. Bart
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h index 86f150c2a6b6..55b25120a1c6 100644 --- a/include/linux/string_helpers.h +++ b/include/linux/string_helpers.h @@ -94,4 +94,6 @@ char *kstrdup_quotable(const char *src, gfp_t gfp); char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp); char *kstrdup_quotable_file(struct file *file, gfp_t gfp); +void kfree_strarray(char **str_array, size_t num_str); + #endif diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 963050c0283e..bfa4c9f3ca0a 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -649,3 +649,28 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp) return pathname; } EXPORT_SYMBOL_GPL(kstrdup_quotable_file); + +/** + * kfree_strarray - free a number of dynamically allocated strings contained + * in an array and the array itself + * + * @str_array: Dynamically allocated array of strings to free. If NULL - the + * function does nothing. + * @num_str: Number of strings (starting from the beginning of the array) to + * free. + * + * Passing a non-null str_array and num_str == 0 as well as NULL str_array + * are valid use-cases. + */ +void kfree_strarray(char **str_array, size_t num_str) +{ + unsigned int i; + + if (!str_array) + return; + + for (i = 0; i < num_str; i++) + kfree(str_array[i]); + kfree(str_array); +} +EXPORT_SYMBOL_GPL(kfree_strarray);