lkml.org 
[lkml]   [2019]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3] media: si2168: Refactor command setup code
From
Date
On 12/07/2019 19:45, Mauro Carvalho Chehab wrote:

> Brad Love <brad@nextdimension.cc> escreveu:
>
>> On 04/07/2019 05.33, Marc Gonzalez wrote:
>>
>>> +#define CMD_SETUP(cmd, args, rlen) \
>>> + cmd_setup(cmd, args, sizeof(args) - 1, rlen)
>>> +
>>
>> This is only a valid helper if args is a null terminated string. It just
>> so happens that every instance in this driver is, but that could be a
>> silent pitfall if someone used a u8 array with this macro.
>
> Actually, it is uglier than that. If one writes something like:
>
> char buf[20];
>
> buf[0] = 0x20;
> buf[1] = 0x03;
>
> CMD_SETUP(cmd, buf, 0);
>
> // some other init, up to 5 values, then another CMD_SETUP()

I'm not sure what you mean in the // comment.
What kind of init? Why up to 5 values? Why another CMD_SETUP?

> sizeof() will evaluate to 20, and not to 2, with would be the
> expected buffer size, and it will pass 18 random values.
>
> IMHO, using sizeof() here is a very bad idea.

You may have a point...
(Though I'm not proposing a kernel API function, merely code
refactoring for a single file that's unlikely to change going
forward.)

It's also bad form to repeat the cmd size (twice) when the compiler
can figure it out automatically for string literals (which is 95%
of the use-cases).

I can drop the macro, and just use the helper...

Or maybe there's a GCC extension to test that an argument is a
string literal...

Regards.

\
 
 \ /
  Last update: 2019-07-13 00:12    [W:0.060 / U:5.428 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site