Messages in this thread | | | Date | Sat, 13 Jul 2019 07:02:56 -0300 | From | Mauro Carvalho Chehab <> | Subject | Re: [PATCH v3] media: si2168: Refactor command setup code |
| |
Em Sat, 13 Jul 2019 00:11:12 +0200 Marc Gonzalez <marc.w.gonzalez@free.fr> escreveu:
> 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?
I mean that the same buffer could be re-used to do something like:
char buf[20];
buf[0] = 0x20; buf[1] = 0x03;
CMD_SETUP(cmd, buf, 0); // write size here should be 2
buf[2] = 0x04 buf[3] = 0x00 buf[4] = 0x05
CMD_SETUP(cmd, buf, 0); // write size here should be 5
This kind of pattern happens on other drivers and someone may end needing something like that at this driver on some future.
> > 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.)
Yes, I know, but we had already some bugs due to the usage of sizeof() on similar macros at drivers in the past.
> 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...
The helper function sounds fine.
> > Or maybe there's a GCC extension to test that an argument is a > string literal...
If this could be evaluated by some advanced macro logic that would work not only with gcc but also with clang, then a macro that does what you proposed could be useful.
There are some ways to check the type of a macro argument, but I'm not sure if are there any way for it to distinguish between a string constant from a char * array.
Thanks, Mauro
| |