All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning
@ 2018-07-11  7:46 Dominique Martinet
  2018-07-11 14:48   ` Chris Wilson
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Dominique Martinet @ 2018-07-11  7:46 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: David Airlie, intel-gfx, dri-devel, linux-kernel, Dominique Martinet

This is effectively no-op as the next line writes a nul at the final
byte of the buffer, so copying one letter less does not change the
behaviour.

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---

gcc 8 gives the following warning, which I am not sure why is treated
as error for this file, thus making me fix it:
drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
   strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


Also, as a side note, while checking if this was already sent I stumbled
uppon this: https://lists.freedesktop.org/archives/intel-gfx/2018-July/170638.html
([Intel-gfx] [PATCH i-g-t 6/7] Fix truncate string in the strncpy)
which replaces strncpy(dest, src, sizeof(dest)) by strncpy(dest, src,
strlen(dest))... This isn't for linux but this looks like a pretty bad
idea to me... (I'm not on the list to reply there, sorry)

 drivers/gpu/drm/i915/intel_tv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index b55b5c157e38..f5614d07b10d 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1355,7 +1355,7 @@ intel_tv_get_modes(struct drm_connector *connector)
 		mode_ptr = drm_mode_create(connector->dev);
 		if (!mode_ptr)
 			continue;
-		strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
+		strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN - 1);
 		mode_ptr->name[DRM_DISPLAY_MODE_LEN - 1] = '\0';
 
 		mode_ptr->hdisplay = hactive_s;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning
  2018-07-11  7:46 [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning Dominique Martinet
@ 2018-07-11 14:48   ` Chris Wilson
  2018-07-11 14:58 ` ✓ Fi.CI.BAT: success for " Patchwork
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2018-07-11 14:48 UTC (permalink / raw)
  To: Dominique Martinet, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: David Airlie, intel-gfx, dri-devel, linux-kernel, Dominique Martinet

Quoting Dominique Martinet (2018-07-11 08:46:15)
> This is effectively no-op as the next line writes a nul at the final
> byte of the buffer, so copying one letter less does not change the
> behaviour.
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
> 
> gcc 8 gives the following warning, which I am not sure why is treated
> as error for this file, thus making me fix it:
> drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
>    strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> 
> Also, as a side note, while checking if this was already sent I stumbled
> uppon this: https://lists.freedesktop.org/archives/intel-gfx/2018-July/170638.html
> ([Intel-gfx] [PATCH i-g-t 6/7] Fix truncate string in the strncpy)
> which replaces strncpy(dest, src, sizeof(dest)) by strncpy(dest, src,
> strlen(dest))... This isn't for linux but this looks like a pretty bad
> idea to me... (I'm not on the list to reply there, sorry)
> 
>  drivers/gpu/drm/i915/intel_tv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index b55b5c157e38..f5614d07b10d 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1355,7 +1355,7 @@ intel_tv_get_modes(struct drm_connector *connector)
>                 mode_ptr = drm_mode_create(connector->dev);
>                 if (!mode_ptr)
>                         continue;
> -               strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> +               strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN - 1);

strlcpy
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning
@ 2018-07-11 14:48   ` Chris Wilson
  0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2018-07-11 14:48 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: David Airlie, intel-gfx, dri-devel, linux-kernel, Dominique Martinet

Quoting Dominique Martinet (2018-07-11 08:46:15)
> This is effectively no-op as the next line writes a nul at the final
> byte of the buffer, so copying one letter less does not change the
> behaviour.
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
> 
> gcc 8 gives the following warning, which I am not sure why is treated
> as error for this file, thus making me fix it:
> drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
>    strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> 
> Also, as a side note, while checking if this was already sent I stumbled
> uppon this: https://lists.freedesktop.org/archives/intel-gfx/2018-July/170638.html
> ([Intel-gfx] [PATCH i-g-t 6/7] Fix truncate string in the strncpy)
> which replaces strncpy(dest, src, sizeof(dest)) by strncpy(dest, src,
> strlen(dest))... This isn't for linux but this looks like a pretty bad
> idea to me... (I'm not on the list to reply there, sorry)
> 
>  drivers/gpu/drm/i915/intel_tv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index b55b5c157e38..f5614d07b10d 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1355,7 +1355,7 @@ intel_tv_get_modes(struct drm_connector *connector)
>                 mode_ptr = drm_mode_create(connector->dev);
>                 if (!mode_ptr)
>                         continue;
> -               strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> +               strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN - 1);

strlcpy
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* ✓ Fi.CI.BAT: success for i915/intel_tv_get_modes: fix strncpy truncation warning
  2018-07-11  7:46 [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning Dominique Martinet
  2018-07-11 14:48   ` Chris Wilson
@ 2018-07-11 14:58 ` Patchwork
  2018-07-11 23:24 ` [PATCH v2] " Dominique Martinet
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-07-11 14:58 UTC (permalink / raw)
  To: Dominique Martinet; +Cc: intel-gfx

== Series Details ==

Series: i915/intel_tv_get_modes: fix strncpy truncation warning
URL   : https://patchwork.freedesktop.org/series/46314/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4470 -> Patchwork_9614 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46314/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9614 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_chamelium@hdmi-hpd-fast:
      fi-kbl-7500u:       SKIP -> FAIL (fdo#103841, fdo#102672)

    
    ==== Warnings ====

    igt@gem_exec_suspend@basic-s4-devices:
      {fi-kbl-8809g}:     DMESG-WARN (fdo#107139) -> INCOMPLETE (fdo#107139)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139


== Participating hosts (46 -> 42) ==

  Additional (1): fi-glk-j4005 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4470 -> Patchwork_9614

  CI_DRM_4470: a8d350f7f795613cfca2428a0535798e14936006 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4549: e19fd5549e9cf603251704117fc64f4068be5016 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9614: 1fde7eeeba1dc99e5da8298c5ecb7c7cc51da112 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

1fde7eeeba1d i915/intel_tv_get_modes: fix strncpy truncation warning

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9614/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] i915/intel_tv_get_modes: fix strncpy truncation warning
  2018-07-11  7:46 [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning Dominique Martinet
  2018-07-11 14:48   ` Chris Wilson
  2018-07-11 14:58 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-07-11 23:24 ` Dominique Martinet
  2018-07-12  7:39     ` Chris Wilson
  2018-07-12  9:19     ` Chris Wilson
  2018-07-12 12:44 ` [Intel-gfx] [PATCH] " Ville Syrjälä
  2018-07-12 23:33 ` ✗ Fi.CI.BAT: failure for i915/intel_tv_get_modes: fix strncpy truncation warning (rev2) Patchwork
  4 siblings, 2 replies; 15+ messages in thread
From: Dominique Martinet @ 2018-07-11 23:24 UTC (permalink / raw)
  To: Chris Wilson, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: David Airlie, intel-gfx, dri-devel, linux-kernel, Dominique Martinet

Change it to use strlcpy instead

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---

 drivers/gpu/drm/i915/intel_tv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index b55b5c157e38..25fee7dba7e2 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1355,8 +1355,7 @@ intel_tv_get_modes(struct drm_connector *connector)
 		mode_ptr = drm_mode_create(connector->dev);
 		if (!mode_ptr)
 			continue;
-		strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
-		mode_ptr->name[DRM_DISPLAY_MODE_LEN - 1] = '\0';
+		strlcpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
 
 		mode_ptr->hdisplay = hactive_s;
 		mode_ptr->hsync_start = hactive_s + 1;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] i915/intel_tv_get_modes: fix strncpy truncation warning
  2018-07-11 23:24 ` [PATCH v2] " Dominique Martinet
@ 2018-07-12  7:39     ` Chris Wilson
  2018-07-12  9:19     ` Chris Wilson
  1 sibling, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2018-07-12  7:39 UTC (permalink / raw)
  To: Dominique Martinet, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: David Airlie, intel-gfx, dri-devel, linux-kernel, Dominique Martinet

Quoting Dominique Martinet (2018-07-12 00:24:46)
> Change it to use strlcpy instead
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] i915/intel_tv_get_modes: fix strncpy truncation warning
@ 2018-07-12  7:39     ` Chris Wilson
  0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2018-07-12  7:39 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: David Airlie, intel-gfx, dri-devel, linux-kernel, Dominique Martinet

Quoting Dominique Martinet (2018-07-12 00:24:46)
> Change it to use strlcpy instead
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] i915/intel_tv_get_modes: fix strncpy truncation warning
  2018-07-11 23:24 ` [PATCH v2] " Dominique Martinet
@ 2018-07-12  9:19     ` Chris Wilson
  2018-07-12  9:19     ` Chris Wilson
  1 sibling, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2018-07-12  9:19 UTC (permalink / raw)
  To: Dominique Martinet, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: David Airlie, intel-gfx, dri-devel, linux-kernel, Dominique Martinet

Quoting Dominique Martinet (2018-07-12 00:24:46)
> Change it to use strlcpy instead
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>

Thanks for the spotting the issue and providing a fix, pushed.
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] i915/intel_tv_get_modes: fix strncpy truncation warning
@ 2018-07-12  9:19     ` Chris Wilson
  0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2018-07-12  9:19 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi
  Cc: David Airlie, intel-gfx, dri-devel, linux-kernel, Dominique Martinet

Quoting Dominique Martinet (2018-07-12 00:24:46)
> Change it to use strlcpy instead
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>

Thanks for the spotting the issue and providing a fix, pushed.
-Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning
  2018-07-11  7:46 [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning Dominique Martinet
                   ` (2 preceding siblings ...)
  2018-07-11 23:24 ` [PATCH v2] " Dominique Martinet
@ 2018-07-12 12:44 ` Ville Syrjälä
  2018-07-12 13:55   ` Dominique Martinet
  2018-07-12 23:33 ` ✗ Fi.CI.BAT: failure for i915/intel_tv_get_modes: fix strncpy truncation warning (rev2) Patchwork
  4 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2018-07-12 12:44 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	intel-gfx, linux-kernel, dri-devel

On Wed, Jul 11, 2018 at 09:46:15AM +0200, Dominique Martinet wrote:
> This is effectively no-op as the next line writes a nul at the final

What is "This". Please write self contained commit messages.

> byte of the buffer, so copying one letter less does not change the
> behaviour.
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
> 
> gcc 8 gives the following warning, which I am not sure why is treated
> as error for this file, thus making me fix it:
> drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
>    strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors

That warning should be in the actual commit message.

> 
> 
> Also, as a side note, while checking if this was already sent I stumbled
> uppon this: https://lists.freedesktop.org/archives/intel-gfx/2018-July/170638.html
> ([Intel-gfx] [PATCH i-g-t 6/7] Fix truncate string in the strncpy)
> which replaces strncpy(dest, src, sizeof(dest)) by strncpy(dest, src,
> strlen(dest))... This isn't for linux but this looks like a pretty bad
> idea to me... (I'm not on the list to reply there, sorry)
> 
>  drivers/gpu/drm/i915/intel_tv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index b55b5c157e38..f5614d07b10d 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1355,7 +1355,7 @@ intel_tv_get_modes(struct drm_connector *connector)
>  		mode_ptr = drm_mode_create(connector->dev);
>  		if (!mode_ptr)
>  			continue;
> -		strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> +		strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN - 1);
>  		mode_ptr->name[DRM_DISPLAY_MODE_LEN - 1] = '\0';

This same pattern is used all over drm. Can you go and fix them all up?
One might even consider writing a cocci patch for it ;)

>  
>  		mode_ptr->hdisplay = hactive_s;
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning
  2018-07-12 12:44 ` [Intel-gfx] [PATCH] " Ville Syrjälä
@ 2018-07-12 13:55   ` Dominique Martinet
  2018-07-12 14:10       ` Ville Syrjälä
  0 siblings, 1 reply; 15+ messages in thread
From: Dominique Martinet @ 2018-07-12 13:55 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	intel-gfx, linux-kernel, dri-devel

Ville Syrjälä wrote on Thu, Jul 12, 2018:
> On Wed, Jul 11, 2018 at 09:46:15AM +0200, Dominique Martinet wrote:
> > This is effectively no-op as the next line writes a nul at the final
> 
> What is "This". Please write self contained commit messages.

This could either be 'this commit' as a whole or if you look only at the
commit message 'this strncpy fix' from the title (which is arguably the
same), and both interpretations sound fairly understandable in the
context of the title line without seeing the patch to me... Although
I'll admit this is difficult to judge of that as the author.

Thanksfully, the v2 of the patch didn't use this wording but while I
agree the message could be better I do not think it was horrible.


> > drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> > drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
> >    strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> >    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> 
> That warning should be in the actual commit message.

Yes and no, I gave it for referrence but when you update to gcc 8 you
will literally see it all over the place.
The words "strncpy truncation warning" is really precise once you've
seen them a few times and there are litteraly hundred of these warnings
in the kernel, some have already been fixed taking a glance at the git
log, some with and without the warning message.
I don't think it's worth polluting the git log with this many
warnings... Which leads to...

> This same pattern is used all over drm. Can you go and fix them all up?
> One might even consider writing a cocci patch for it ;)

Now this is something I can agree with.
This patch really was just a stop-gap measure because I could not build
the kernel at all without it, but yes I did consider having a look at
others.

Unfortunately coccinelle does not run on fedora 28 (and doesn't look
like it will fix itself any time soon, there is a bug report[1] open
since February that didn't get much love lately - I was just looking at
it a few days ago)

I think in this case it might actually be faster to look at gcc warnings
and s/strncpy/strlcpy/, but I am curious about Coccinelle so this is a
good excuse to look at it, I'll report back in a bit after poking at
that bug report and figuring out how coccinelle works.

I do not guarantee speed however, if anyone sees this and feels put off
from donig it themselves, please go ahead and just drop me a word.


[1] https://bugzilla.redhat.com/show_bug.cgi?id=1544204

Thanks, and sorry for the mail longer than I originally intended,
-- 
Dominique Martinet

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning
  2018-07-12 13:55   ` Dominique Martinet
@ 2018-07-12 14:10       ` Ville Syrjälä
  0 siblings, 0 replies; 15+ messages in thread
From: Ville Syrjälä @ 2018-07-12 14:10 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	intel-gfx, linux-kernel, dri-devel

On Thu, Jul 12, 2018 at 03:55:26PM +0200, Dominique Martinet wrote:
> Ville Syrjälä wrote on Thu, Jul 12, 2018:
> > On Wed, Jul 11, 2018 at 09:46:15AM +0200, Dominique Martinet wrote:
> > > This is effectively no-op as the next line writes a nul at the final
> > 
> > What is "This". Please write self contained commit messages.
> 
> This could either be 'this commit' as a whole or if you look only at the
> commit message 'this strncpy fix' from the title (which is arguably the
> same), and both interpretations sound fairly understandable in the
> context of the title line without seeing the patch to me... Although
> I'll admit this is difficult to judge of that as the author.

The patch subject is not part of the commit message body though. This is
made all the more clear when I'm editing the response in vim that doesn't
even show the mail subject to me. Hence I'm always left in the dark by
commit messages that aren't fully self contained.

> 
> Thanksfully, the v2 of the patch didn't use this wording but while I
> agree the message could be better I do not think it was horrible.
> 
> 
> > > drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> > > drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
> > >    strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> > >    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > 
> > That warning should be in the actual commit message.
> 
> Yes and no, I gave it for referrence but when you update to gcc 8 you
> will literally see it all over the place.
> The words "strncpy truncation warning" is really precise once you've
> seen them a few times and there are litteraly hundred of these warnings
> in the kernel, some have already been fixed taking a glance at the git
> log, some with and without the warning message.
> I don't think it's worth polluting the git log with this many
> warnings... Which leads to...

I disagree. Without knowing what exactly is fixed how can you judge 
whether the patch even makes sense? And later you may get another
report of the same warning and then you would want to look through
the git log to see if there's a patch that already fixed it. Quite
hard to do without the exact warning in the log.

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning
@ 2018-07-12 14:10       ` Ville Syrjälä
  0 siblings, 0 replies; 15+ messages in thread
From: Ville Syrjälä @ 2018-07-12 14:10 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: David Airlie, intel-gfx, linux-kernel, dri-devel, Rodrigo Vivi

On Thu, Jul 12, 2018 at 03:55:26PM +0200, Dominique Martinet wrote:
> Ville Syrjälä wrote on Thu, Jul 12, 2018:
> > On Wed, Jul 11, 2018 at 09:46:15AM +0200, Dominique Martinet wrote:
> > > This is effectively no-op as the next line writes a nul at the final
> > 
> > What is "This". Please write self contained commit messages.
> 
> This could either be 'this commit' as a whole or if you look only at the
> commit message 'this strncpy fix' from the title (which is arguably the
> same), and both interpretations sound fairly understandable in the
> context of the title line without seeing the patch to me... Although
> I'll admit this is difficult to judge of that as the author.

The patch subject is not part of the commit message body though. This is
made all the more clear when I'm editing the response in vim that doesn't
even show the mail subject to me. Hence I'm always left in the dark by
commit messages that aren't fully self contained.

> 
> Thanksfully, the v2 of the patch didn't use this wording but while I
> agree the message could be better I do not think it was horrible.
> 
> 
> > > drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> > > drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
> > >    strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> > >    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > 
> > That warning should be in the actual commit message.
> 
> Yes and no, I gave it for referrence but when you update to gcc 8 you
> will literally see it all over the place.
> The words "strncpy truncation warning" is really precise once you've
> seen them a few times and there are litteraly hundred of these warnings
> in the kernel, some have already been fixed taking a glance at the git
> log, some with and without the warning message.
> I don't think it's worth polluting the git log with this many
> warnings... Which leads to...

I disagree. Without knowing what exactly is fixed how can you judge 
whether the patch even makes sense? And later you may get another
report of the same warning and then you would want to look through
the git log to see if there's a patch that already fixed it. Quite
hard to do without the exact warning in the log.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Intel-gfx] [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning
  2018-07-12 14:10       ` Ville Syrjälä
  (?)
@ 2018-07-12 14:30       ` Dominique Martinet
  -1 siblings, 0 replies; 15+ messages in thread
From: Dominique Martinet @ 2018-07-12 14:30 UTC (permalink / raw)
  To: Ville Syrjälä
  Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, David Airlie,
	intel-gfx, linux-kernel, dri-devel

Ville Syrjälä wrote on Thu, Jul 12, 2018:
> On Thu, Jul 12, 2018 at 03:55:26PM +0200, Dominique Martinet wrote:
> > This could either be 'this commit' as a whole or if you look only at the
> > commit message 'this strncpy fix' from the title (which is arguably the
> > same), and both interpretations sound fairly understandable in the
> > context of the title line without seeing the patch to me... Although
> > I'll admit this is difficult to judge of that as the author.
> 
> The patch subject is not part of the commit message body though. This is
> made all the more clear when I'm editing the response in vim that doesn't
> even show the mail subject to me. Hence I'm always left in the dark by
> commit messages that aren't fully self contained.

Ah, that is a fair point - I thought you were referring to the patch
itself, not the subject. My mail client does include the subject in the
editor so I hadn't considered that, but I understand where you come from
now and agree.
I will be more mindful of that as the v2 has the same problem.


> > Yes and no, I gave it for referrence but when you update to gcc 8 you
> > will literally see it all over the place.
> > The words "strncpy truncation warning" is really precise once you've
> > seen them a few times and there are litteraly hundred of these warnings
> > in the kernel, some have already been fixed taking a glance at the git
> > log, some with and without the warning message.
> > I don't think it's worth polluting the git log with this many
> > warnings... Which leads to...
> 
> I disagree. Without knowing what exactly is fixed how can you judge 
> whether the patch even makes sense? And later you may get another
> report of the same warning and then you would want to look through
> the git log to see if there's a patch that already fixed it. Quite
> hard to do without the exact warning in the log.

I might just be tired of this specific warning; I've fixed it countless
times in different projects these past few months and it's coming out of
my eyes at this point.

I definitely agree in general, just -Wstringop-truncation has been
showing up everywhere and it's always the same, with many occurences I
don't consider to be bugs (like here because we forcefully terminate the
last byte of the string afterwards), so it's really lost value to me.

I included it as a comment precisely for your first point (so you can tell
the patch makes sense now) but I do not feel any regret not recording
it, and I still stand by what I said: if all you want is examples of
patches that already fix it, I've just had a look at git log in drm
trees and there already have been many fixes, most of which provided a
warning similar to the one I got.

Attaching the full warning messages makes sense if the warning is
new/rare but if it's the same as 5 other commits in semi-recent history
I do not see much point.

Anyway, I would be enclined to add it just to comply now but it looks
like Chris already picked the v2 up, so there is not much point in
arguing, sorry for disagreeing.

-- 
Dominique Martinet

^ permalink raw reply	[flat|nested] 15+ messages in thread

* ✗ Fi.CI.BAT: failure for i915/intel_tv_get_modes: fix strncpy truncation warning (rev2)
  2018-07-11  7:46 [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning Dominique Martinet
                   ` (3 preceding siblings ...)
  2018-07-12 12:44 ` [Intel-gfx] [PATCH] " Ville Syrjälä
@ 2018-07-12 23:33 ` Patchwork
  4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-07-12 23:33 UTC (permalink / raw)
  To: Dominique Martinet; +Cc: intel-gfx

== Series Details ==

Series: i915/intel_tv_get_modes: fix strncpy truncation warning (rev2)
URL   : https://patchwork.freedesktop.org/series/46314/
State : failure

== Summary ==

Applying: i915/intel_tv_get_modes: fix strncpy truncation warning
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_tv.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-07-12 23:33 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-11  7:46 [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning Dominique Martinet
2018-07-11 14:48 ` Chris Wilson
2018-07-11 14:48   ` Chris Wilson
2018-07-11 14:58 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-07-11 23:24 ` [PATCH v2] " Dominique Martinet
2018-07-12  7:39   ` Chris Wilson
2018-07-12  7:39     ` Chris Wilson
2018-07-12  9:19   ` Chris Wilson
2018-07-12  9:19     ` Chris Wilson
2018-07-12 12:44 ` [Intel-gfx] [PATCH] " Ville Syrjälä
2018-07-12 13:55   ` Dominique Martinet
2018-07-12 14:10     ` Ville Syrjälä
2018-07-12 14:10       ` Ville Syrjälä
2018-07-12 14:30       ` [Intel-gfx] " Dominique Martinet
2018-07-12 23:33 ` ✗ Fi.CI.BAT: failure for i915/intel_tv_get_modes: fix strncpy truncation warning (rev2) Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.