| Message ID | 20180827082101.5036-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] devres: provide devm_kstrdup_const() | expand |
On Mon, 2018-08-27 at 10:21 +0200, Bartosz Golaszewski wrote: > Provide a resource managed version of kstrdup_const(). This variant > internally calls devm_kstrdup() on pointers that are outside of > .rodata section. Also provide a corresponding version of devm_kfree(). [] > diff --git a/mm/util.c b/mm/util.c [] > /** > * kstrdup - allocate space for and copy an existing string > * @s: the string to duplicate > @@ -78,6 +92,27 @@ const char *kstrdup_const(const char *s, gfp_t gfp) > } > EXPORT_SYMBOL(kstrdup_const); > > +/** > + * devm_kstrdup_const - resource managed conditional string duplication > + * @dev: device for which to duplicate the string > + * @s: the string to duplicate > + * @gfp: the GFP mask used in the kmalloc() call when allocating memory > + * > + * Function returns source string if it is in .rodata section otherwise it > + * fallbacks to devm_kstrdup. > + * > + * Strings allocated by devm_kstrdup_const will be automatically freed when > + * the associated device is detached. > + */ > +char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp) > +{ > + if (is_kernel_rodata((unsigned long)s)) > + return s; > + > + return devm_kstrdup(dev, s, gfp); > +} > +EXPORT_SYMBOL(devm_kstrdup_const); Doesn't this lose constness and don't you get a compiler warning here? The kstrdup_const function returns a const char *, why shouldn't this?
2018-08-27 10:42 GMT+02:00 Joe Perches <joe@perches.com>: > On Mon, 2018-08-27 at 10:21 +0200, Bartosz Golaszewski wrote: >> Provide a resource managed version of kstrdup_const(). This variant >> internally calls devm_kstrdup() on pointers that are outside of >> .rodata section. Also provide a corresponding version of devm_kfree(). > [] >> diff --git a/mm/util.c b/mm/util.c > [] >> /** >> * kstrdup - allocate space for and copy an existing string >> * @s: the string to duplicate >> @@ -78,6 +92,27 @@ const char *kstrdup_const(const char *s, gfp_t gfp) >> } >> EXPORT_SYMBOL(kstrdup_const); >> >> +/** >> + * devm_kstrdup_const - resource managed conditional string duplication >> + * @dev: device for which to duplicate the string >> + * @s: the string to duplicate >> + * @gfp: the GFP mask used in the kmalloc() call when allocating memory >> + * >> + * Function returns source string if it is in .rodata section otherwise it >> + * fallbacks to devm_kstrdup. >> + * >> + * Strings allocated by devm_kstrdup_const will be automatically freed when >> + * the associated device is detached. >> + */ >> +char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp) >> +{ >> + if (is_kernel_rodata((unsigned long)s)) >> + return s; >> + >> + return devm_kstrdup(dev, s, gfp); >> +} >> +EXPORT_SYMBOL(devm_kstrdup_const); > > Doesn't this lose constness and don't you get > a compiler warning here? > Yes it does but for some reason gcc 6.3 didn't complain... > The kstrdup_const function returns a const char *, > why shouldn't this? > It probably should, I'll fix it for v2. Bart
On Mon, Aug 27, 2018 at 10:21:00AM +0200, Bartosz Golaszewski wrote: > Provide a resource managed version of kstrdup_const(). This variant > internally calls devm_kstrdup() on pointers that are outside of > .rodata section. Also provide a corresponding version of devm_kfree(). > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > --- > include/linux/device.h | 2 ++ > mm/util.c | 35 +++++++++++++++++++++++++++++++++++ > 2 files changed, 37 insertions(+) > > diff --git a/include/linux/device.h b/include/linux/device.h > index 8f882549edee..f8f5982d26b2 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -693,7 +693,9 @@ static inline void *devm_kcalloc(struct device *dev, > return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO); > } > extern void devm_kfree(struct device *dev, void *p); > +extern void devm_kfree_const(struct device *dev, void *p); > extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc; > +extern char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp); > extern void *devm_kmemdup(struct device *dev, const void *src, size_t len, > gfp_t gfp); > > diff --git a/mm/util.c b/mm/util.c > index d2890a407332..6d1f41b5775e 100644 > --- a/mm/util.c > +++ b/mm/util.c > @@ -39,6 +39,20 @@ void kfree_const(const void *x) > } > EXPORT_SYMBOL(kfree_const); > > +/** > + * devm_kfree_const - Resource managed conditional kfree > + * @dev: device this memory belongs to > + * @p: memory to free > + * > + * Function calls devm_kfree only if @p is not in .rodata section. > + */ > +void devm_kfree_const(struct device *dev, void *p) > +{ > + if (!is_kernel_rodata((unsigned long)p)) > + devm_kfree(dev, p); > +} > +EXPORT_SYMBOL(devm_kfree_const); > + > /** > * kstrdup - allocate space for and copy an existing string > * @s: the string to duplicate > @@ -78,6 +92,27 @@ const char *kstrdup_const(const char *s, gfp_t gfp) > } > EXPORT_SYMBOL(kstrdup_const); > > +/** > + * devm_kstrdup_const - resource managed conditional string duplication > + * @dev: device for which to duplicate the string > + * @s: the string to duplicate > + * @gfp: the GFP mask used in the kmalloc() call when allocating memory > + * > + * Function returns source string if it is in .rodata section otherwise it > + * fallbacks to devm_kstrdup. Please make it proper "Returns:" description and move to the end of the comment. See Documentation/doc-guide/kernel-doc.rst. > + * Strings allocated by devm_kstrdup_const will be automatically freed when > + * the associated device is detached. > + */ > +char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp) > +{ > + if (is_kernel_rodata((unsigned long)s)) > + return s; > + > + return devm_kstrdup(dev, s, gfp); > +} > +EXPORT_SYMBOL(devm_kstrdup_const); > + The devm_ variants seem to belong to drivers/base/devres.c rather than mm/util.c > /** > * kstrndup - allocate space for and copy an existing string > * @s: the string to duplicate > -- > 2.18.0 >
Hi Bartosz,
I love your patch! Perhaps something to improve:
[auto build test WARNING on clk/clk-next]
[also build test WARNING on v4.19-rc1 next-20180827]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/devres-provide-devm_kstrdup_const/20180827-162336
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: powerpc-mpc836x_mds_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=powerpc
All warnings (new ones prefixed by >>):
mm/util.c: In function 'devm_kstrdup_const':
>> mm/util.c:110:10: warning: return discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
return s;
^
vim +/const +110 mm/util.c
94
95 /**
96 * devm_kstrdup_const - resource managed conditional string duplication
97 * @dev: device for which to duplicate the string
98 * @s: the string to duplicate
99 * @gfp: the GFP mask used in the kmalloc() call when allocating memory
100 *
101 * Function returns source string if it is in .rodata section otherwise it
102 * fallbacks to devm_kstrdup.
103 *
104 * Strings allocated by devm_kstrdup_const will be automatically freed when
105 * the associated device is detached.
106 */
107 char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp)
108 {
109 if (is_kernel_rodata((unsigned long)s))
> 110 return s;
111
112 return devm_kstrdup(dev, s, gfp);
113 }
114 EXPORT_SYMBOL(devm_kstrdup_const);
115
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
2018-08-27 12:33 GMT+02:00 Mike Rapoport <rppt@linux.vnet.ibm.com>: > On Mon, Aug 27, 2018 at 10:21:00AM +0200, Bartosz Golaszewski wrote: >> Provide a resource managed version of kstrdup_const(). This variant >> internally calls devm_kstrdup() on pointers that are outside of >> .rodata section. Also provide a corresponding version of devm_kfree(). >> >> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> >> --- >> include/linux/device.h | 2 ++ >> mm/util.c | 35 +++++++++++++++++++++++++++++++++++ >> 2 files changed, 37 insertions(+) >> >> diff --git a/include/linux/device.h b/include/linux/device.h >> index 8f882549edee..f8f5982d26b2 100644 >> --- a/include/linux/device.h >> +++ b/include/linux/device.h >> @@ -693,7 +693,9 @@ static inline void *devm_kcalloc(struct device *dev, >> return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO); >> } >> extern void devm_kfree(struct device *dev, void *p); >> +extern void devm_kfree_const(struct device *dev, void *p); >> extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc; >> +extern char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp); >> extern void *devm_kmemdup(struct device *dev, const void *src, size_t len, >> gfp_t gfp); >> >> diff --git a/mm/util.c b/mm/util.c >> index d2890a407332..6d1f41b5775e 100644 >> --- a/mm/util.c >> +++ b/mm/util.c >> @@ -39,6 +39,20 @@ void kfree_const(const void *x) >> } >> EXPORT_SYMBOL(kfree_const); >> >> +/** >> + * devm_kfree_const - Resource managed conditional kfree >> + * @dev: device this memory belongs to >> + * @p: memory to free >> + * >> + * Function calls devm_kfree only if @p is not in .rodata section. >> + */ >> +void devm_kfree_const(struct device *dev, void *p) >> +{ >> + if (!is_kernel_rodata((unsigned long)p)) >> + devm_kfree(dev, p); >> +} >> +EXPORT_SYMBOL(devm_kfree_const); >> + >> /** >> * kstrdup - allocate space for and copy an existing string >> * @s: the string to duplicate >> @@ -78,6 +92,27 @@ const char *kstrdup_const(const char *s, gfp_t gfp) >> } >> EXPORT_SYMBOL(kstrdup_const); >> >> +/** >> + * devm_kstrdup_const - resource managed conditional string duplication >> + * @dev: device for which to duplicate the string >> + * @s: the string to duplicate >> + * @gfp: the GFP mask used in the kmalloc() call when allocating memory >> + * >> + * Function returns source string if it is in .rodata section otherwise it >> + * fallbacks to devm_kstrdup. > > Please make it proper "Returns:" description and move to the end of the > comment. See Documentation/doc-guide/kernel-doc.rst. > Sure. >> + * Strings allocated by devm_kstrdup_const will be automatically freed when >> + * the associated device is detached. >> + */ >> +char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp) >> +{ >> + if (is_kernel_rodata((unsigned long)s)) >> + return s; >> + >> + return devm_kstrdup(dev, s, gfp); >> +} >> +EXPORT_SYMBOL(devm_kstrdup_const); >> + > > The devm_ variants seem to belong to drivers/base/devres.c rather than > mm/util.c > Not all devm_ variants live in drivers/base/devres.c, many subsystems implement them locally. In this case we need to choose between exporting is_kernel_rodata() and putting devm_kstrdup_const() in mm/util.c. I chose the latter, since it's cleaner. Bart >> /** >> * kstrndup - allocate space for and copy an existing string >> * @s: the string to duplicate >> -- >> 2.18.0 >> > > -- > Sincerely yours, > Mike. >
On Mon, Aug 27, 2018 at 04:28:55PM +0200, Bartosz Golaszewski wrote: > 2018-08-27 12:33 GMT+02:00 Mike Rapoport <rppt@linux.vnet.ibm.com>: > > On Mon, Aug 27, 2018 at 10:21:00AM +0200, Bartosz Golaszewski wrote: > >> Provide a resource managed version of kstrdup_const(). This variant > >> internally calls devm_kstrdup() on pointers that are outside of > >> .rodata section. Also provide a corresponding version of devm_kfree(). > >> > >> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > >> --- > >> include/linux/device.h | 2 ++ > >> mm/util.c | 35 +++++++++++++++++++++++++++++++++++ > >> 2 files changed, 37 insertions(+) > >> > >> diff --git a/include/linux/device.h b/include/linux/device.h > >> index 8f882549edee..f8f5982d26b2 100644 > >> --- a/include/linux/device.h > >> +++ b/include/linux/device.h > >> @@ -693,7 +693,9 @@ static inline void *devm_kcalloc(struct device *dev, > >> return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO); > >> } > >> extern void devm_kfree(struct device *dev, void *p); > >> +extern void devm_kfree_const(struct device *dev, void *p); > >> extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc; > >> +extern char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp); > >> extern void *devm_kmemdup(struct device *dev, const void *src, size_t len, > >> gfp_t gfp); > >> > >> diff --git a/mm/util.c b/mm/util.c > >> index d2890a407332..6d1f41b5775e 100644 > >> --- a/mm/util.c > >> +++ b/mm/util.c [ ... ] > >> + * Strings allocated by devm_kstrdup_const will be automatically freed when > >> + * the associated device is detached. > >> + */ > >> +char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp) > >> +{ > >> + if (is_kernel_rodata((unsigned long)s)) > >> + return s; > >> + > >> + return devm_kstrdup(dev, s, gfp); > >> +} > >> +EXPORT_SYMBOL(devm_kstrdup_const); > >> + > > > > The devm_ variants seem to belong to drivers/base/devres.c rather than > > mm/util.c > > > > Not all devm_ variants live in drivers/base/devres.c, many subsystems > implement them locally. In this case we need to choose between > exporting is_kernel_rodata() and putting devm_kstrdup_const() in > mm/util.c. I chose the latter, since it's cleaner. I rather think that moving is_kernel_rodata() to include/asm-generic/sections.h and having devm_kstrdup_const() next to devm_kstrdup() would be cleaner. > Bart >
diff --git a/include/linux/device.h b/include/linux/device.h index 8f882549edee..f8f5982d26b2 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -693,7 +693,9 @@ static inline void *devm_kcalloc(struct device *dev, return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO); } extern void devm_kfree(struct device *dev, void *p); +extern void devm_kfree_const(struct device *dev, void *p); extern char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc; +extern char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp); extern void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp); diff --git a/mm/util.c b/mm/util.c index d2890a407332..6d1f41b5775e 100644 --- a/mm/util.c +++ b/mm/util.c @@ -39,6 +39,20 @@ void kfree_const(const void *x) } EXPORT_SYMBOL(kfree_const); +/** + * devm_kfree_const - Resource managed conditional kfree + * @dev: device this memory belongs to + * @p: memory to free + * + * Function calls devm_kfree only if @p is not in .rodata section. + */ +void devm_kfree_const(struct device *dev, void *p) +{ + if (!is_kernel_rodata((unsigned long)p)) + devm_kfree(dev, p); +} +EXPORT_SYMBOL(devm_kfree_const); + /** * kstrdup - allocate space for and copy an existing string * @s: the string to duplicate @@ -78,6 +92,27 @@ const char *kstrdup_const(const char *s, gfp_t gfp) } EXPORT_SYMBOL(kstrdup_const); +/** + * devm_kstrdup_const - resource managed conditional string duplication + * @dev: device for which to duplicate the string + * @s: the string to duplicate + * @gfp: the GFP mask used in the kmalloc() call when allocating memory + * + * Function returns source string if it is in .rodata section otherwise it + * fallbacks to devm_kstrdup. + * + * Strings allocated by devm_kstrdup_const will be automatically freed when + * the associated device is detached. + */ +char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp) +{ + if (is_kernel_rodata((unsigned long)s)) + return s; + + return devm_kstrdup(dev, s, gfp); +} +EXPORT_SYMBOL(devm_kstrdup_const); + /** * kstrndup - allocate space for and copy an existing string * @s: the string to duplicate
Provide a resource managed version of kstrdup_const(). This variant internally calls devm_kstrdup() on pointers that are outside of .rodata section. Also provide a corresponding version of devm_kfree(). Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> --- include/linux/device.h | 2 ++ mm/util.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+)