Subject: Re: [PATCH] drm: move allocation out of drm_get_format_name()
To: Rob Clark <robdclark@gmail.com>, Eric Engestrom <eric@engestrom.ch>
References: <20161105012344.GA28349@engestrom.ch>
 <20161105013325.3889-1-eric@engestrom.ch>
 <cc7acbea-a721-22b4-67d3-22198decced6@amd.com>
 <20161105163844.GA29546@engestrom.ch>
 <CAF6AEGuEB82fjZ_YF61MAAqU=8ahXuMo06MN0_ZuWY5fkyHnoQ@mail.gmail.com>
CC: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "Wei Yongjun" <yongjun_wei@trendmicro.com.cn>,
        Daniel Vetter <daniel.vetter@intel.com>, Flora Cui <Flora.Cui@amd.com>,
        Gustavo Padovan <gustavo.padovan@collabora.co.uk>,
        Tom St Denis <tom.stdenis@amd.com>,
        "Thomas Hellstrom" <thellstrom@vmware.com>,
        Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
        Xinliang Liu <z.liuxinliang@hisilicon.com>,
        VMware Graphics <linux-graphics-maintainer@vmware.com>,
        Vitaly Prosyak <vitaly.prosyak@amd.com>,
        Alexandre Demers <alexandre.f.demers@gmail.com>,
        Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
        Emily Deng <Emily.Deng@amd.com>, Ken Wang <Qingqing.Wang@amd.com>,
        Junwei Zhang <Jerry.Zhang@amd.com>,
        =?UTF-8?Q?Michel_D=c3=a4nzer?= <michel.daenzer@amd.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alex Deucher <alexander.deucher@amd.com>,
        Colin Ian King <colin.king@canonical.com>
From: =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Message-ID: <8af873a2-31a9-1146-289d-35d2e48edffa@amd.com>
Date: Sun, 6 Nov 2016 10:47:25 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <CAF6AEGuEB82fjZ_YF61MAAqU=8ahXuMo06MN0_ZuWY5fkyHnoQ@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Nov 2016 09:47:35.7482
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR12MB1298
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2231
Lines: 57

Am 05.11.2016 um 17:49 schrieb Rob Clark:
> On Sat, Nov 5, 2016 at 12:38 PM, Eric Engestrom <eric@engestrom.ch> 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?

Regards,
Christian.

>
> BR,
> -R
>
>> I can rewrite the typedef out if you think it's better.
>>
>> Cheers,
>>    Eric
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel