Re: [PATCH] drm: move allocation out of drm_get_format_name()

From: Christian KÃnig
Date: Sun Nov 06 2016 - 04:47:55 EST

Am 05.11.2016 um 17:49 schrieb Rob Clark:
On Sat, Nov 5, 2016 at 12:38 PM, Eric Engestrom <eric@xxxxxxxxxxxx> wrote:
On Saturday, 2016-11-05 13:11:36 +0100, Christian KÃnig wrote:
Am 05.11.2016 um 02:33 schrieb Eric Engestrom:
+typedef char drm_format_name_buf[32];
Please don't use a typedef for this, just define the maximum size of
characters the function might write somewhere.

See the kernel coding style as well:
In general, a pointer, or a struct that has elements that can reasonably
be directly accessed should **never** be a typedef.
I would normally agree as I tend to hate typedefs ($DAYJOB {ab,mis}uses
them way too much), and your way was what I wrote at first, but Rob Clark's
typedef idea makes it much harder for someone to allocate a buffer of
the wrong size, which IMO is good thing here.
IMHO I would make a small test program to verify this actually helps
the compiler catch problems. And if it does, I would stick with it.
The coding-style should be guidelines, not something that supersedes
common sense / practicality.

Well completely agree that we should be able to question the coding style rules, but when we do it we discuss this on a the mailing list first and then start to use it in code. Not the other way around.

That is my $0.02 anyways.. if others vehemently disagree and want to
dogmatically stick to the coding-style guidelines, ok then. OTOH, if
this approach doesn't help the compiler catch issues, then it isn't
worth it.

Yeah, exactly that's the point. If I'm not completely mistaken the compiler won't issue a warning here if you pass an array with the wrong size.

I think you need something like "struct drm_format_name_buf { char str[32]; };" to trigger this.

Apart from that is this function really called so often that using kasprintf() is a problem here? Or is there another motivation behind the change?



I can rewrite the typedef out if you think it's better.

dri-devel mailing list