Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581988 > unrolled thread

[PATCH] drm: drm_printer: add __printf validation

Back to article view | Back to linux.kernel

Contents

  [PATCH] drm: drm_printer: add __printf validation Joe Perches <joe@perches.com> - 2017-02-16 00:40 +0100
    Re: [PATCH] drm: drm_printer: add __printf validation Eric Engestrom <eric.engestrom@imgtec.com> - 2017-02-20 13:40 +0100
      Re: [PATCH] drm: drm_printer: add __printf validation Joe Perches <joe@perches.com> - 2017-02-20 18:20 +0100
        Re: [PATCH] drm: drm_printer: add __printf validation Jani Nikula <jani.nikula@linux.intel.com> - 2017-02-21 09:30 +0100
          Re: [PATCH] drm: drm_printer: add __printf validation Joe Perches <joe@perches.com> - 2017-02-21 09:40 +0100
            Re: [PATCH] drm: drm_printer: add __printf validation Jani Nikula <jani.nikula@linux.intel.com> - 2017-02-21 10:10 +0100
              Re: [PATCH] drm: drm_printer: add __printf validation Joe Perches <joe@perches.com> - 2017-02-21 10:20 +0100
                Re: [PATCH] drm: drm_printer: add __printf validation Daniel Vetter <daniel@ffwll.ch> - 2017-02-26 21:50 +0100

#1581988 — [PATCH] drm: drm_printer: add __printf validation

drm_printf does not currently use the compiler to verify
format and arguments.  Make it do so.

Miscellanea:

o Add appropriate #include files for __printf and struct va_format
o Convert dev_printk to dev_info

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/gpu/drm/drm_print.c | 2 +-
 include/drm/drm_print.h     | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 02a107d50706..74c466aca622 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -36,7 +36,7 @@ EXPORT_SYMBOL(__drm_printfn_seq_file);
 
 void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf)
 {
-	dev_printk(KERN_INFO, p->arg, "[" DRM_NAME "] %pV", vaf);
+	dev_info(p->arg, "[" DRM_NAME "] %pV", vaf);
 }
 EXPORT_SYMBOL(__drm_printfn_info);
 
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 7d98763c0444..ca4d7c6321f2 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -26,6 +26,8 @@
 #ifndef DRM_PRINT_H_
 #define DRM_PRINT_H_
 
+#include <linux/compiler.h>
+#include <linux/printk.h>
 #include <linux/seq_file.h>
 #include <linux/device.h>
 
@@ -75,6 +77,7 @@ void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf);
 void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);
 
+__printf(2, 3)
 void drm_printf(struct drm_printer *p, const char *f, ...);
 
 
-- 
2.10.0.rc2.1.g053435c

[toc] | [next] | [standalone]

#1584576

On Wednesday, 2017-02-15 15:33:18 -0800, Joe Perches wrote:
> drm_printf does not currently use the compiler to verify
> format and arguments.  Make it do so.
> 
> Miscellanea:
> 
> o Add appropriate #include files for __printf and struct va_format
> o Convert dev_printk to dev_info

I think these unrelated changes should be in 4 patches:
1 - add annotation to check the format string against the arguments
    (linux/compiler.h should be added here)
2 - add missing linux/printk.h header for struct va_format
    Note that I think a forward declaration is more appropriate here, as
    we only use pointers to this struct in this file, we never try to
    look inside. On the other hand:
3 - drm_print.c needs the header in drm_printf(), but as a separate
    patch
4 - convert dev_printk to dev_info (you need to include linux/device.h
    there)

You can add my r-b on all four patches when you send them to the list :)

Cheers,
  Eric

> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/gpu/drm/drm_print.c | 2 +-
>  include/drm/drm_print.h     | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index 02a107d50706..74c466aca622 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -36,7 +36,7 @@ EXPORT_SYMBOL(__drm_printfn_seq_file);
>  
>  void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf)
>  {
> -	dev_printk(KERN_INFO, p->arg, "[" DRM_NAME "] %pV", vaf);
> +	dev_info(p->arg, "[" DRM_NAME "] %pV", vaf);
>  }
>  EXPORT_SYMBOL(__drm_printfn_info);
>  
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 7d98763c0444..ca4d7c6321f2 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -26,6 +26,8 @@
>  #ifndef DRM_PRINT_H_
>  #define DRM_PRINT_H_
>  
> +#include <linux/compiler.h>
> +#include <linux/printk.h>
>  #include <linux/seq_file.h>
>  #include <linux/device.h>
>  
> @@ -75,6 +77,7 @@ void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf);
>  void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf);
>  void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);
>  
> +__printf(2, 3)
>  void drm_printf(struct drm_printer *p, const char *f, ...);
>  
>  
> -- 
> 2.10.0.rc2.1.g053435c
> 

[toc] | [prev] | [next] | [standalone]

#1584778

On Mon, 2017-02-20 at 12:17 +0000, Eric Engestrom wrote:
> On Wednesday, 2017-02-15 15:33:18 -0800, Joe Perches wrote:
> > drm_printf does not currently use the compiler to verify
> > format and arguments.  Make it do so.
> > 
> > Miscellanea:
> > 
> > o Add appropriate #include files for __printf and struct va_format
> > o Convert dev_printk to dev_info
> 
> I think these unrelated changes should be in 4 patches:
> 1 - add annotation to check the format string against the arguments
>     (linux/compiler.h should be added here)
> 2 - add missing linux/printk.h header for struct va_format
>     Note that I think a forward declaration is more appropriate here, as
>     we only use pointers to this struct in this file, we never try to
>     look inside. On the other hand:
> 3 - drm_print.c needs the header in drm_printf(), but as a separate
>     patch
> 4 - convert dev_printk to dev_info (you need to include linux/device.h
>     there)

I am not a big fan of making trivial
patches into a series.

> You can add my r-b on all four patches when you send them to the list :)

If you want to break it up, go ahead.

[toc] | [prev] | [next] | [standalone]

#1585121

On Mon, 20 Feb 2017, Joe Perches <joe@perches.com> wrote:
> On Mon, 2017-02-20 at 12:17 +0000, Eric Engestrom wrote:
>> On Wednesday, 2017-02-15 15:33:18 -0800, Joe Perches wrote:
>> > drm_printf does not currently use the compiler to verify
>> > format and arguments.  Make it do so.
>> > 
>> > Miscellanea:
>> > 
>> > o Add appropriate #include files for __printf and struct va_format
>> > o Convert dev_printk to dev_info
>> 
>> I think these unrelated changes should be in 4 patches:
>> 1 - add annotation to check the format string against the arguments
>>     (linux/compiler.h should be added here)
>> 2 - add missing linux/printk.h header for struct va_format
>>     Note that I think a forward declaration is more appropriate here, as
>>     we only use pointers to this struct in this file, we never try to
>>     look inside. On the other hand:
>> 3 - drm_print.c needs the header in drm_printf(), but as a separate
>>     patch
>> 4 - convert dev_printk to dev_info (you need to include linux/device.h
>>     there)
>
> I am not a big fan of making trivial patches into a series.

It's standard procedure in kernel development to split out unrelated
changes into individual patches, regardless of whether you think they
are trivial or not. Four is probably excessive, but you get the idea.

>> You can add my r-b on all four patches when you send them to the list :)
>
> If you want to break it up, go ahead.

You know how this stuff works, please split it up to get the stuff
merged.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1585125

On Tue, 2017-02-21 at 10:26 +0200, Jani Nikula wrote:
> You know how this stuff works, please split it up to get the stuff
> merged.

Quite well actually.

Fix it as you think appropriate.
But in any case, fix it.

[toc] | [prev] | [next] | [standalone]

#1585140

On Tue, 21 Feb 2017, Joe Perches <joe@perches.com> wrote:
> On Tue, 2017-02-21 at 10:26 +0200, Jani Nikula wrote:
>> You know how this stuff works, please split it up to get the stuff
>> merged.
>
> Quite well actually.
>
> Fix it as you think appropriate.
> But in any case, fix it.

Yes, I'm sure someone will eventually send patches I can apply.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1585154

On Tue, 2017-02-21 at 11:02 +0200, Jani Nikula wrote:
> On Tue, 21 Feb 2017, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2017-02-21 at 10:26 +0200, Jani Nikula wrote:
> > > You know how this stuff works, please split it up to get the stuff
> > > merged.
> > 
> > Quite well actually.
> > 
> > Fix it as you think appropriate.
> > But in any case, fix it.
> 
> Yes, I'm sure someone will eventually send patches I can apply.

As you know it's a defect, you or one
of the other maintainers of that file
should fix it and not wait for what you
consider the ideal patch.

cheers, Joe

[toc] | [prev] | [next] | [standalone]

#1588395

On Tue, Feb 21, 2017 at 01:18:03AM -0800, Joe Perches wrote:
> On Tue, 2017-02-21 at 11:02 +0200, Jani Nikula wrote:
> > On Tue, 21 Feb 2017, Joe Perches <joe@perches.com> wrote:
> > > On Tue, 2017-02-21 at 10:26 +0200, Jani Nikula wrote:
> > > > You know how this stuff works, please split it up to get the stuff
> > > > merged.
> > > 
> > > Quite well actually.
> > > 
> > > Fix it as you think appropriate.
> > > But in any case, fix it.
> > 
> > Yes, I'm sure someone will eventually send patches I can apply.
> 
> As you know it's a defect, you or one
> of the other maintainers of that file
> should fix it and not wait for what you
> consider the ideal patch.

Yeah, requesting a split-up of this patch seems like a genuine bikeshed.
Merged to drm-misc for 4.12, thanks a lot for your patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web