Re: [PATCH v3 4/8] drm/print: Add drm_printf_indent()

[Jani Nikula]

On Thu, 26 Oct 2017, Ville Syrjälä  wrote:
> On Thu, Oct 26, 2017 at 08:51:57PM +0200, Noralf Trønnes wrote:
>> 
>> Den 26.10.2017 19.49, skrev Ville Syrjälä:
>> > On Thu, Oct 26, 2017 at 06:57:27PM +0200, Noralf Trønnes wrote:
>> >> Add drm_printf_indent() that adds tab indentation according to argument.
>> >>
>> >> Signed-off-by: Noralf Trønnes 
>> >> ---
>> >>   drivers/gpu/drm/drm_print.c | 21 +
>> >>   include/drm/drm_print.h |  2 ++
>> >>   2 files changed, 23 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
>> >> index 0b3bf476dc4b..dac3ee98b30f 100644
>> >> --- a/drivers/gpu/drm/drm_print.c
>> >> +++ b/drivers/gpu/drm/drm_print.c
>> >> @@ -64,6 +64,27 @@ void drm_printf(struct drm_printer *p, const char *f, 
>> >> ...)
>> >>   }
>> >>   EXPORT_SYMBOL(drm_printf);
>> >>   
>> >> +/**
>> >> + * drm_printf_indent - print to a _printer stream with indentation
>> >> + * @p: the _printer
>> >> + * @i: indentation
>> >> + * @f: format string
>> >> + */
>> >> +void drm_printf_indent(struct drm_printer *p, unsigned int i, const char 
>> >> *f, ...)
>> >> +{
>> >> + struct va_format vaf;
>> >> + va_list args;
>> >> +
>> >> + drm_printf(p, "%.*s", i, "\t\t\t\t\t\t\t\t\t\t");
>> >> +
>> >> + va_start(args, f);
>> >> + vaf.fmt = f;
>> >> + vaf.va = 
>> >> + p->printfn(p, );
>> >> + va_end(args);
>> >> +}
>> >> +EXPORT_SYMBOL(drm_printf_indent);
>> > The double printf() is rather unfortunate. Sadly I don't think there's
>> > any proper way to manipulate a va_list to avoid that.
>> >
>> > Hmm. Would it work if we simply make it a macro? Eg.
>> >
>> > #define drm_printf_indent(printer, indent, fmt, ...) \
>> >drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t", ##__VA_ARGS__)
>> 
>> The macro worked fine and it looks like a better solution to me.
>
> Only downside is that it bloats every format string. But as long
> as there aren't a huge number of useless callers we should be fine.
>
>> 
>> > The "\t\t\t..." thing is also rather off putting, but I guess it's
>> > the best we can do if we want to keep it to one printf(). And maybe we
>> > should have a check in there to make sure we have enought tabs in the
>> > string to satisfy the indent level, or we just clamp the indent level
>> > silently to something reasonable?
>> 
>> I put 10 tabs in my suggestion, which should be enough and I think it's
>> OK to just silently fail to do more. If 10 isn't enough it's easy to add
>> more for the developer that hits the limit.
>
> 10 is probably even overkill. I'd say if you have to go past four or so
> then there's alredy a bigger problem in the code ;)

Just put some non-tab character at the end of the tab string? Dunno, +
or * or something, just to draw attention to it instead of just silently
failing to indent.

Overall I think this is a neat way to do indents.

BR,
Jani.


>
>> 
>> Noralf.
>> 
>> > Oh, seeing the \t now reminds me that tabs won't necesarily get printed
>> > out properly. At least I've seen fbcon just printing some weird blobs
>> > instead of tabs. Not sure if it's just a matter of having a crappy font
>> > or what. That said, the state dump code is using tabs already, so I guess
>> > this wouldn't make it worse at least.
>> >
>> >> +
>> >>   #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"
>> >>   
>> >>   void drm_dev_printk(const struct device *dev, const char *level,
>> >> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
>> >> index 7b9c86a6ca3e..73dcd16eca49 100644
>> >> --- a/include/drm/drm_print.h
>> >> +++ b/include/drm/drm_print.h
>> >> @@ -79,6 +79,8 @@ 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, ...);
>> >> +__printf(3, 4)
>> >> +void drm_printf_indent(struct drm_printer *p, unsigned int i, const char 
>> >> *f, ...);
>> >>   
>> >>   
>> >>   /**
>> >> -- 
>> >> 2.14.2

-- 
Jani Nikula, Intel Open Source Technology Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/8] drm/print: Add drm_printf_indent()

[Ville Syrjälä]

On Thu, Oct 26, 2017 at 08:51:57PM +0200, Noralf Trønnes wrote:
> 
> Den 26.10.2017 19.49, skrev Ville Syrjälä:
> > On Thu, Oct 26, 2017 at 06:57:27PM +0200, Noralf Trønnes wrote:
> >> Add drm_printf_indent() that adds tab indentation according to argument.
> >>
> >> Signed-off-by: Noralf Trønnes 
> >> ---
> >>   drivers/gpu/drm/drm_print.c | 21 +
> >>   include/drm/drm_print.h |  2 ++
> >>   2 files changed, 23 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> >> index 0b3bf476dc4b..dac3ee98b30f 100644
> >> --- a/drivers/gpu/drm/drm_print.c
> >> +++ b/drivers/gpu/drm/drm_print.c
> >> @@ -64,6 +64,27 @@ void drm_printf(struct drm_printer *p, const char *f, 
> >> ...)
> >>   }
> >>   EXPORT_SYMBOL(drm_printf);
> >>   
> >> +/**
> >> + * drm_printf_indent - print to a _printer stream with indentation
> >> + * @p: the _printer
> >> + * @i: indentation
> >> + * @f: format string
> >> + */
> >> +void drm_printf_indent(struct drm_printer *p, unsigned int i, const char 
> >> *f, ...)
> >> +{
> >> +  struct va_format vaf;
> >> +  va_list args;
> >> +
> >> +  drm_printf(p, "%.*s", i, "\t\t\t\t\t\t\t\t\t\t");
> >> +
> >> +  va_start(args, f);
> >> +  vaf.fmt = f;
> >> +  vaf.va = 
> >> +  p->printfn(p, );
> >> +  va_end(args);
> >> +}
> >> +EXPORT_SYMBOL(drm_printf_indent);
> > The double printf() is rather unfortunate. Sadly I don't think there's
> > any proper way to manipulate a va_list to avoid that.
> >
> > Hmm. Would it work if we simply make it a macro? Eg.
> >
> > #define drm_printf_indent(printer, indent, fmt, ...) \
> > drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t", ##__VA_ARGS__)
> 
> The macro worked fine and it looks like a better solution to me.

Only downside is that it bloats every format string. But as long
as there aren't a huge number of useless callers we should be fine.

> 
> > The "\t\t\t..." thing is also rather off putting, but I guess it's
> > the best we can do if we want to keep it to one printf(). And maybe we
> > should have a check in there to make sure we have enought tabs in the
> > string to satisfy the indent level, or we just clamp the indent level
> > silently to something reasonable?
> 
> I put 10 tabs in my suggestion, which should be enough and I think it's
> OK to just silently fail to do more. If 10 isn't enough it's easy to add
> more for the developer that hits the limit.

10 is probably even overkill. I'd say if you have to go past four or so
then there's alredy a bigger problem in the code ;)

> 
> Noralf.
> 
> > Oh, seeing the \t now reminds me that tabs won't necesarily get printed
> > out properly. At least I've seen fbcon just printing some weird blobs
> > instead of tabs. Not sure if it's just a matter of having a crappy font
> > or what. That said, the state dump code is using tabs already, so I guess
> > this wouldn't make it worse at least.
> >
> >> +
> >>   #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"
> >>   
> >>   void drm_dev_printk(const struct device *dev, const char *level,
> >> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> >> index 7b9c86a6ca3e..73dcd16eca49 100644
> >> --- a/include/drm/drm_print.h
> >> +++ b/include/drm/drm_print.h
> >> @@ -79,6 +79,8 @@ 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, ...);
> >> +__printf(3, 4)
> >> +void drm_printf_indent(struct drm_printer *p, unsigned int i, const char 
> >> *f, ...);
> >>   
> >>   
> >>   /**
> >> -- 
> >> 2.14.2

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/8] drm/print: Add drm_printf_indent()

[Noralf Trønnes]

Den 26.10.2017 19.49, skrev Ville Syrjälä:

On Thu, Oct 26, 2017 at 06:57:27PM +0200, Noralf Trønnes wrote:

Add drm_printf_indent() that adds tab indentation according to argument.

Signed-off-by: Noralf Trønnes 
---
  drivers/gpu/drm/drm_print.c | 21 +
  include/drm/drm_print.h |  2 ++
  2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 0b3bf476dc4b..dac3ee98b30f 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -64,6 +64,27 @@ void drm_printf(struct drm_printer *p, const char *f, ...)
  }
  EXPORT_SYMBOL(drm_printf);
  
+/**

+ * drm_printf_indent - print to a _printer stream with indentation
+ * @p: the _printer
+ * @i: indentation
+ * @f: format string
+ */
+void drm_printf_indent(struct drm_printer *p, unsigned int i, const char *f, 
...)
+{
+   struct va_format vaf;
+   va_list args;
+
+   drm_printf(p, "%.*s", i, "\t\t\t\t\t\t\t\t\t\t");
+
+   va_start(args, f);
+   vaf.fmt = f;
+   vaf.va = 
+   p->printfn(p, );
+   va_end(args);
+}
+EXPORT_SYMBOL(drm_printf_indent);

The double printf() is rather unfortunate. Sadly I don't think there's
any proper way to manipulate a va_list to avoid that.

Hmm. Would it work if we simply make it a macro? Eg.

#define drm_printf_indent(printer, indent, fmt, ...) \
drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t", ##__VA_ARGS__)


The macro worked fine and it looks like a better solution to me.


The "\t\t\t..." thing is also rather off putting, but I guess it's
the best we can do if we want to keep it to one printf(). And maybe we
should have a check in there to make sure we have enought tabs in the
string to satisfy the indent level, or we just clamp the indent level
silently to something reasonable?


I put 10 tabs in my suggestion, which should be enough and I think it's
OK to just silently fail to do more. If 10 isn't enough it's easy to add
more for the developer that hits the limit.

Noralf.


Oh, seeing the \t now reminds me that tabs won't necesarily get printed
out properly. At least I've seen fbcon just printing some weird blobs
instead of tabs. Not sure if it's just a matter of having a crappy font
or what. That said, the state dump code is using tabs already, so I guess
this wouldn't make it worse at least.


+
  #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"
  
  void drm_dev_printk(const struct device *dev, const char *level,

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 7b9c86a6ca3e..73dcd16eca49 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -79,6 +79,8 @@ 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, ...);
+__printf(3, 4)
+void drm_printf_indent(struct drm_printer *p, unsigned int i, const char *f, 
...);
  
  
  /**

--
2.14.2


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/8] drm/print: Add drm_printf_indent()

[Ville Syrjälä]

On Thu, Oct 26, 2017 at 06:57:27PM +0200, Noralf Trønnes wrote:
> Add drm_printf_indent() that adds tab indentation according to argument.
> 
> Signed-off-by: Noralf Trønnes 
> ---
>  drivers/gpu/drm/drm_print.c | 21 +
>  include/drm/drm_print.h |  2 ++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index 0b3bf476dc4b..dac3ee98b30f 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -64,6 +64,27 @@ void drm_printf(struct drm_printer *p, const char *f, ...)
>  }
>  EXPORT_SYMBOL(drm_printf);
>  
> +/**
> + * drm_printf_indent - print to a _printer stream with indentation
> + * @p: the _printer
> + * @i: indentation
> + * @f: format string
> + */
> +void drm_printf_indent(struct drm_printer *p, unsigned int i, const char *f, 
> ...)
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + drm_printf(p, "%.*s", i, "\t\t\t\t\t\t\t\t\t\t");
> +
> + va_start(args, f);
> + vaf.fmt = f;
> + vaf.va = 
> + p->printfn(p, );
> + va_end(args);
> +}
> +EXPORT_SYMBOL(drm_printf_indent);

The double printf() is rather unfortunate. Sadly I don't think there's
any proper way to manipulate a va_list to avoid that.

Hmm. Would it work if we simply make it a macro? Eg.

#define drm_printf_indent(printer, indent, fmt, ...) \
drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t", ##__VA_ARGS__)

The "\t\t\t..." thing is also rather off putting, but I guess it's
the best we can do if we want to keep it to one printf(). And maybe we
should have a check in there to make sure we have enought tabs in the
string to satisfy the indent level, or we just clamp the indent level
silently to something reasonable?

Oh, seeing the \t now reminds me that tabs won't necesarily get printed
out properly. At least I've seen fbcon just printing some weird blobs
instead of tabs. Not sure if it's just a matter of having a crappy font
or what. That said, the state dump code is using tabs already, so I guess
this wouldn't make it worse at least.

> +
>  #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"
>  
>  void drm_dev_printk(const struct device *dev, const char *level,
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 7b9c86a6ca3e..73dcd16eca49 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -79,6 +79,8 @@ 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, ...);
> +__printf(3, 4)
> +void drm_printf_indent(struct drm_printer *p, unsigned int i, const char *f, 
> ...);
>  
>  
>  /**
> -- 
> 2.14.2

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel