Em Sat, 13 Jul 2019 00:11:12 +0200 Marc Gonzalez <marc.w.gonzalez@xxxxxxx> escreveu: > On 12/07/2019 19:45, Mauro Carvalho Chehab wrote: > > > Brad Love <brad@xxxxxxxxxxxxxxxx> 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
