[PATCH] lib/string_helpers: add linux/string.h for strlen()

[PATCH] lib/string_helpers: add linux/string.h for strlen()

[Lucas De Marchi]

linux/string_helpers.h uses strlen(), so include the correpondent
header. Otherwise we get a compilation error if it's not also included
by whoever included the helper.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 include/linux/string_helpers.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 68189c4a2eb1..4ba39e1403b2 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -4,6 +4,7 @@
 
 #include <linux/bits.h>
 #include <linux/ctype.h>
+#include <linux/string.h>
 #include <linux/types.h>
 
 struct file;
-- 
2.33.0

Re: [PATCH] lib/string_helpers: add linux/string.h for strlen()

[Andrew Morton]

On Tue,  5 Oct 2021 14:26:34 -0700 Lucas De Marchi <lucas.demarchi@intel.com> wrote:

> linux/string_helpers.h uses strlen(), so include the correpondent
> header. Otherwise we get a compilation error if it's not also included
> by whoever included the helper.

Is such a compilation error demonstrable in the current mainline kernel?

Re: [PATCH] lib/string_helpers: add linux/string.h for strlen()

[Lucas De Marchi]

On Tue, Oct 05, 2021 at 04:21:21PM -0700, Andrew Morton wrote:
>On Tue,  5 Oct 2021 14:26:34 -0700 Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
>> linux/string_helpers.h uses strlen(), so include the correpondent
>> header. Otherwise we get a compilation error if it's not also included
>> by whoever included the helper.
>
>Is such a compilation error demonstrable in the current mainline kernel?

No, not with the current mainline. I was just starting to implement some
more helpers there and noticed the issue when including this header from
i915. Then I noticed  Andy and Jani already had similar patches to what
I was doing:

https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko@linux.intel.com/
and https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula@intel.com/

So I'm following up on the first thread abovee to figure out what would
be the proper header to implement this. Meanwhile, we can have this
quick harmless fix.

thanks,
Lucas De Marchi

Re: [PATCH] lib/string_helpers: add linux/string.h for strlen()

[Lucas De Marchi]

On Wed, Oct 06, 2021 at 08:57:27AM +0300, Andy Shevchenko wrote:
>On Wednesday, October 6, 2021, Lucas De Marchi <lucas.demarchi@intel.com>
>wrote:
>
>> linux/string_helpers.h uses strlen(), so include the correpondent
>> header. Otherwise we get a compilation error if it's not also included
>> by whoever included the helper.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  include/linux/string_helpers.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/string_helpers.h b/include/linux/string_
>> helpers.h
>> index 68189c4a2eb1..4ba39e1403b2 100644
>> --- a/include/linux/string_helpers.h
>> +++ b/include/linux/string_helpers.h
>> @@ -4,6 +4,7 @@
>>
>>  #include <linux/bits.h>
>>  #include <linux/ctype.h>
>> +#include <linux/string.h>
>>  #include <linux/types.h>
>
>
>I’m afraid this potentially can add into header dependencies hell. What
>about moving the user to the C file?

I can do that, but I don't see the problem here... afaics it has been like this
for 7 years, since commit c8250381c827 ("lib / string_helpers: introduce string_escape_mem()"),
and the only way it was never borken is because
linux/string.h is already being indirectly included from other headers.
So just adding it here is harmless.

I reproduced this while following the normal header order in i915 and
adding linux/string_helpers.h like this:


diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 309d74fd86ce..1dfc01617258 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -3,6 +3,8 @@
   * Copyright © 2020 Intel Corporation
   */
  
+#include <linux/string_helpers.h>
+
  #include <drm/drm_debugfs.h>
  #include <drm/drm_fourcc.h>
  

Note that this became the first header included, producing the following
error:

  make -j$(nproc) drivers/gpu/drm/i915/display/intel_display_debugfs.o
   DESCEND objtool
   CALL    scripts/atomic/check-atomics.sh
   CALL    scripts/checksyscalls.sh
   CC [M]  drivers/gpu/drm/i915/display/intel_display_debugfs.o
In file included from drivers/gpu/drm/i915/display/intel_display_debugfs.c:6:
./include/linux/string_helpers.h: In function ‘string_escape_str’:
./include/linux/string_helpers.h:75:32: error: implicit declaration of function ‘strlen’ [-Werror=implicit-function-declaration]
    75 |  return string_escape_mem(src, strlen(src), dst, sz, flags, only);
       |                                ^~~~~~
./include/linux/string_helpers.h:75:32: error: incompatible implicit declaration of built-in function ‘strlen’ [-Werror]
./include/linux/string_helpers.h:7:1: note: include ‘<string.h>’ or provide a declaration of ‘strlen’
     6 | #include <linux/ctype.h>
   +++ |+#include <string.h>
     7 | #include <linux/types.h>
cc1: all warnings being treated as errors


Anyway, if it's preferable to move these functions out of line, I can do
so.

thanks
Lucas De Marchi

>
>
>>
>>  struct file;
>> --
>> 2.33.0
>>
>>
>
>-- 
>With Best Regards,
>Andy Shevchenko

Re: [PATCH] lib/string_helpers: add linux/string.h for strlen()

[Andy Shevchenko]

On Wed, Oct 6, 2021 at 9:30 AM Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Wed, Oct 06, 2021 at 08:57:27AM +0300, Andy Shevchenko wrote:
> >On Wednesday, October 6, 2021, Lucas De Marchi <lucas.demarchi@intel.com>
> >wrote:
> >
> >> linux/string_helpers.h uses strlen(), so include the correspondent

correspondent

> >> header. Otherwise we get a compilation error if it's not also included
> >> by whoever included the helper.

...

> >I’m afraid this potentially can add into header dependencies hell. What
> >about moving the user to the C file?
>
> I can do that, but I don't see the problem here... afaics it has been like this
> for 7 years, since commit c8250381c827 ("lib / string_helpers: introduce string_escape_mem()"),
> and the only way it was never borken is because
> linux/string.h is already being indirectly included from other headers.

> So just adding it here is harmless.

Quite possible.

...

> Anyway, if it's preferable to move these functions out of line, I can do
> so.

I have no strong opinion, whatever maintainers will decide then!

-- 
With Best Regards,
Andy Shevchenko