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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 26 Oct 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> 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 <noralf@tronnes.org>
>> >> ---
>> >>   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 &drm_printer stream with indentation
>> >> + * @p: the &drm_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 = &args;
>> >> +	p->printfn(p, &vaf);
>> >> +	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