linux-kernel - Re: [PATCH] staging: media: atomisp: Add parentheses around macro definitions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cf999a28-7227-4ee9-bc5f-9fe8c370458c@suswa.mountain>
Date: Sat, 3 Aug 2024 01:10:58 -0500
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Sean Whitton <spwhitton@...hitton.name>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	~lkcamp/patches@...ts.sr.ht, helen.koike@...labora.com,
	Hans de Goede <hdegoede@...hat.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	linux-media@...r.kernel.org, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: media: atomisp: Add parentheses around macro
 definitions

On Sat, Aug 03, 2024 at 01:33:43PM +0800, Sean Whitton wrote:
> Hello,
> 
> On Fri 02 Aug 2024 at 11:28pm -05, Dan Carpenter wrote:
> 
> > *You* need to figure out what the proper thing is.  Not us.  That's the
> > difficult part of writing a patch.  Once you know what the correct thing
> > is, then the rest is just typing.
> >
> > That business of defining STORAGE_CLASS_SP_C is weird.  Figure out the
> > authors intention and find a better way to do it.
> >
> > Figure out why your code compiled as well because putting parentheses
> > around (static inline) is a syntax error.
> 
> I asked follow-up questions because it seems like at least partially a
> matter of style to say that the business of defining STORAGE_CLASS_SP_C
> is weird.

I'm a domain expert when it comes to kernel style.  ;)  Trust me, it's
weird.  There are other places which do it as will but it's not normal.

> Maybe there is a better approach than what is currently done,
> but maybe there isn't.

Correct.  Just because it's weird, doesn't mean it's wrong.  Figure out
why the author did what they did and after that you'll probably be able
to judge if it makes sense.

> Maybe the checkpatch warning should just be
> suppressed (if that's something that can be done).

Yes.  Try to suppress the warning.  You don't need anyone's permission.
I think it will be difficult and I doubt you will succeed.  But you
never know until you try.  Even if you don't succeed, it's a useful
exercise.

> I would be grateful for some additional pointers.
> 

Ok.  Here was your question.

> I don't know what the author's intention was.  Are you saying that you
> think this preprocessor mechanism should just be replaced with
> hardcoding 'extern' or 'static inline' in each file which includes one
> of these headers?

The answer is you need to figure out what the author's intention was.
1) Look through the git log.  2) Try removing it and see if anything
breaks.  3) Do a grep for __INLINE_SP__.  (I deleted some extra hints
here because if I give any more hints then it's just me doing the
project).

Once you know why the macro exists then you can decide it we should do a
sed to replace it.  The sed to get rid of the macro is just an automated
one liner thing.  The difficult part is answering why the macro was
created and do we still need it?

regards,
dan carpenter