Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Marc Gonzalez <marc.w.gonzalez@free.fr>
To: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
	Brad Love <brad@nextdimension.cc>
Cc: "Antti Palosaari" <crope@iki.fi>,
	"Jonathan Neuschäfer" <j.neuschaefer@gmx.net>,
	linux-media <linux-media@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	MSM <linux-arm-msm@vger.kernel.org>,
	"Bjorn Andersson" <bjorn.andersson@linaro.org>
Subject: Re: [PATCH v3] media: si2168: Refactor command setup code
Date: Sat, 13 Jul 2019 00:11:12 +0200
Message-ID: <10f064c5-1634-c9f9-fcc9-6ab51b7f8f0b@free.fr> (raw)
In-Reply-To: <20190712144537.2bad2482@coco.lan>

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.

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-04 10:33 Marc Gonzalez
2019-07-12  8:43 ` Uwe Kleine-König
2019-07-12  9:37   ` Marc Gonzalez
2019-07-12 15:47 ` Brad Love
2019-07-12 17:45   ` Mauro Carvalho Chehab
2019-07-12 22:11     ` Marc Gonzalez [this message]
2019-07-13 10:02       ` Mauro Carvalho Chehab
2019-07-14 18:31         ` Matthias Schwarzott
2019-07-12 21:41   ` Marc Gonzalez

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=10f064c5-1634-c9f9-fcc9-6ab51b7f8f0b@free.fr \
    --to=marc.w.gonzalez@free.fr \
    --cc=bjorn.andersson@linaro.org \
    --cc=brad@nextdimension.cc \
    --cc=crope@iki.fi \
    --cc=j.neuschaefer@gmx.net \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab+samsung@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox